Skip to content

migrated some part of test always to pytest - #29375

Merged
potiuk merged 5 commits into
apache:mainfrom
Abhishek-kumar-ISM:migrating_test_always_to_pytest
Feb 12, 2023
Merged

migrated some part of test always to pytest#29375
potiuk merged 5 commits into
apache:mainfrom
Abhishek-kumar-ISM:migrating_test_always_to_pytest

Conversation

@Abhishek-kumar-ISM

Copy link
Copy Markdown
Contributor

Migrated some part of tests/always to pytest

related: #29305

@Abhishek-kumar-ISM

Copy link
Copy Markdown
Contributor Author

@Taragolis migrated some tests/always to pytest.

Please review.

@Taragolis Taragolis added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) type:misc/internal Changelog: Misc changes that should appear in change log labels Feb 7, 2023
@Taragolis

Copy link
Copy Markdown
Contributor

You could check previous PR with migration always to pytest. Potentially you could find useful information here

@Taragolis Taragolis mentioned this pull request Feb 7, 2023
13 tasks
@Abhishek-kumar-ISM

Copy link
Copy Markdown
Contributor Author

@Taragolis ok i will try to see.
Actually tests/always has two files which need to be migrated to pytest.
In one file they are using self.subTest which was giving me problem while migrating, and other file in @parameterized.expand a function was passed which was giving some problem to me while migrating.

So i converted only a part of tests/always.

But i will try to cover other left part also.

@Taragolis

Copy link
Copy Markdown
Contributor

In one file they are using self.subTest which was giving me problem while migrating,

As far as I remember:

  1. In always subtest is useless and could be replaced by regular assert. And this test is broken and always success
  2. In operators subtest also broken (do not cleanup stage between subtest run). In general all subtest could be rewrited by parametrised tests (@pytest.mark.parametrize)

So i converted only a part of tests/always.

No problem, there is no target to do everything in one go

@Taragolis

Copy link
Copy Markdown
Contributor

However still better to fix at list one module 😉

@Taragolis
Taragolis force-pushed the migrating_test_always_to_pytest branch from 37bff89 to 231222d Compare February 7, 2023 15:59
@Abhiaps

Abhiaps commented Feb 7, 2023

Copy link
Copy Markdown

In one file they are using self.subTest which was giving me problem while migrating,

As far as I remember:

  1. In always subtest is useless and could be replaced by regular assert. And this test is broken and always success
  2. In operators subtest also broken (do not cleanup stage between subtest run). In general all subtest could be rewrited by parametrised tests (@pytest.mark.parametrize)

So i converted only a part of tests/always.

No problem, there is no target to do everything in one go

😊Okay i will try

@Abhishek-kumar-ISM

Abhishek-kumar-ISM commented Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

@Taragolis migrated all tests/always files to pytest.

I didn't find any alternative of mock objects of unittest, so i haven't done anything with mock objects of unittest,

Only changed @parameterized.expand to @pytest.mark.parametrize, changed subtest to some alternative and other small changes.

@Abhishek-kumar-ISM

Copy link
Copy Markdown
Contributor Author

@Taragolis @potiuk @eladkal all checks have passed.

Please review.

Comment thread tests/always/test_project_structure.py Outdated
missing_tests_files = expected_test_files - expected_test_files.intersection(current_test_files)

with self.subTest("Detect missing tests in providers module"):
try:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That looks strange. I am not sure if I understand why we have try/Except at all (assert with description should be enough) and I am not sure what was the effect of self.subTest here. Can you please explain @Abhishek-kumar-samsung ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained just below.

@potiuk potiuk Feb 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Makes sense for subtest removal.

But why not just this:

assert set() == missing_tests_files, "Detect missing tests in providers module"

@Abhishek-kumar-ISM Abhishek-kumar-ISM Feb 12, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't knew if it can be done that way, i knew try except way, and tried testing by putting missing_tests_files as none, try except way was working fine so i kept that.
And also as per understanding, try except was much easy to understand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want, i can try your way @potiuk.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh okay, i will modify it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And no. For me in this case try/except is actually confusing, because it makes the person reading it to think "is there any reason why this is done this way? Did I miss something?".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk done as you have said.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for a record this test was broken before, and missing_tests_files always empty set.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well.. :)

@Abhiaps

Abhiaps commented Feb 12, 2023

Copy link
Copy Markdown

@potiuk Actually as per what I read subTest can be used if we need to do multiple tests with varying arguments. But here it was just a single test which can be written via simple assert only.

So I am not sure why they used subTest here.
In one more PR in which I migrated tests/operator test functions, there were several uses of subTest which makes a lot of sense, there they used for loop to create several subTests with varying arguments.

But here I don't think it makes much sense of writing subTest if we can simply write assert.

Just above in this PR only @Taragolis also commented that here subTest is useless and can be easily replaced by regular assert.

@Abhishek-kumar-ISM

Copy link
Copy Markdown
Contributor Author

@potiuk i have added the changes as you have requested, and it is working fine, checks are also passed.

@potiuk
potiuk merged commit 19316da into apache:main Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) type:misc/internal Changelog: Misc changes that should appear in change log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants