Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test suite optimization: Migrate tests to pytest #917

Closed
wants to merge 25 commits into from

Conversation

Naman-Priyadarshi
Copy link
Member

No description provided.

@Naman-Priyadarshi Naman-Priyadarshi marked this pull request as draft June 30, 2023 08:39
@Naman-Priyadarshi Naman-Priyadarshi self-assigned this Jun 30, 2023
Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

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

Looks good to me so far! 👍 Thanks!

Do you have any open questions / things to discuss?

benchexec/test_analyze_run_result.py Outdated Show resolved Hide resolved
benchexec/test_analyze_run_result.py Outdated Show resolved Hide resolved
from benchexec.util import ProcessExitCode
import tempfile
import os
import stat
import shutil
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to use shell utilities as I was getting errors multiple time i.e. FAILED test_util.py::TestRmtree::test_writable_file - AssertionError: Failed to remove directory with file FAILED test_util.py::TestRmtree::test_writable_dir - AssertionError: Failed to remove directory with child directory FAILED test_util.py::TestRmtree::test_nonwritable_file - AssertionError: Failed to remove directory with non-writable file

@Naman-Priyadarshi
Copy link
Member Author

Hi @PhilippWendler !, according to the Appveyor service I think we need to migrate the tests inside benchexec.tablegenerator first to make sure the pipeline does not fail, let me know if I'm correct or not

@PhilippWendler
Copy link
Member

Hi @PhilippWendler !, according to the Appveyor service I think we need to migrate the tests inside benchexec.tablegenerator first to make sure the pipeline does not fail, let me know if I'm correct or not

Not sure what you are referring to here, but of course we want to migrate all tests anyway, so just pick the order that makes sense.

@Naman-Priyadarshi
Copy link
Member Author

Hi @PhilippWendler, wanted some guidance related the tests test_no_fail.py and test_with_fail.py. I have installed ptf in my system and still getting the error no module named ptf

@PhilippWendler
Copy link
Member

These tests are test for addon modules, that are not part of BenchExec per se. They require specific installation instructions. The tests are currently not run as part of the BenchExec test suite, and I think we should keep it this ways.

@Naman-Priyadarshi
Copy link
Member Author

I'm getting specific errors in the test files in /benchexec/tablegenerator which seems to be a cache related problem according to me (can be wrong). The errors can be found at the gitlab jobs page. The error is only present for the table generator tests and not the other ones. The error is present even when I'm running a basic test with assert 1=1.

@PhilippWendler
Copy link
Member

Do you mean the failing assert decimal.getcontext().rounding == decimal.ROUND_HALF_UP? This has nothing to do with a cache. The decimal package has a context that table-generator tries to reconfigure when it is imported. However, if the decimal package was already imported and used previously by something else, this might be too late, and then the assertion triggers. So we either need to make sure that nothing uses decimal earlier or configure decimal appropriately before.

@Naman-Priyadarshi
Copy link
Member Author

you must've missed the repeating error, the error seems to be <frozen importlib._bootstrap> which keeps repeating for all the 3 test files present inside the tablegenerator folder. Also I haven't pushed the test_columns.py globally that's why the decimal package error is present.

@PhilippWendler
Copy link
Member

What error do you mean? Please give exact details, like the line number of the log. I only see <frozen importlib._bootstrap> as part of the three stack traces of the failing decimal.getcontext().rounding == decimal.ROUND_HALF_UP assertions, but not as an error in itself.

@Naman-Priyadarshi
Copy link
Member Author

Naman-Priyadarshi commented Oct 17, 2023

Do you mean the failing assert decimal.getcontext().rounding == decimal.ROUND_HALF_UP? This has nothing to do with a cache. The decimal package has a context that table-generator tries to reconfigure when it is imported. However, if the decimal package was already imported and used previously by something else, this might be too late, and then the assertion triggers. So we either need to make sure that nothing uses decimal earlier or configure decimal appropriately before.

Turns out this is the issue itself, I misunderstood the traces as an error 🙃

@Naman-Priyadarshi
Copy link
Member Author

@PhilippWendler, I wish to complete this project as soon as possible and I think I would require some help from your side to help solve this problem

@PhilippWendler
Copy link
Member

PhilippWendler commented Oct 30, 2023

What question do you have? What have you attempted so far? Why didn't it work?

@Naman-Priyadarshi
Copy link
Member Author

What question do you have? What have you attempted so far? Why didn't it work?

Currently I haven't worked on the issue yet, I wanted to ask how we could configure decimal or make sure that it's not used before the assertion?

@PhilippWendler
Copy link
Member

I guess the usage of decimal is by pytest? One could trace it probably, there should be some way to call Python and debug the imports and their order.

If it is ok for pytest's usage of decimal to also use our configured rounding mode, you could see whether you can add some configuration/setup code for pytest that gets applied before the tests are run.

@Naman-Priyadarshi
Copy link
Member Author

I guess the usage of decimal is by pytest? One could trace it probably, there should be some way to call Python and debug the imports and their order.

If it is ok for pytest's usage of decimal to also use our configured rounding mode, you could see whether you can add some configuration/setup code for pytest that gets applied before the tests are run.

so I tried some different methods to solve our issue here and all of them led to no success.
only one seems to work but it brings in the factor that we need to remove assert decimal.getcontext().rounding == decimal.ROUND_HALF_UP which removes the double checking factor in columns.py when we configure decimals. I also created a conftest.py file which sets both DefaultContext and local context rounding to ROUND_HALF_UP at the beginning of the testing session.

I am pretty sure that method is not suitable for our issue here but I would love to get your insights about it

@PhilippWendler
Copy link
Member

Well, then I guess we can just set the correct rounding context for the thread that executes the test, right? I.e., change decimal.getcontext().rounding.

@Naman-Priyadarshi
Copy link
Member Author

yes we can do that by using our conftest.py file which gets executed before the tests our run, i'll push the changes so you can see the effects

@Naman-Priyadarshi
Copy link
Member Author

so the tests and workflow seems to be working now except that the docker tests test_no_fail.py and test_with_fail are also getting executed. Now I'll work on a way how to ignore these two test files

Copy link
Member

@PhilippWendler PhilippWendler left a comment

Choose a reason for hiding this comment

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

I see that pytest seems to execute much fewer tests than nose: compare 386 tests with nose against 310 tests. What is the reason for this?

The CI logs also indicate that nose is still being installed during testing.

decimal.DefaultContext.rounding = decimal.ROUND_HALF_UP
assert decimal.getcontext().rounding == decimal.ROUND_HALF_UP
Copy link
Member

Choose a reason for hiding this comment

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

I thought this can be kept now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried multiple ways so that we can still keep the double-checking assertion but the tests only work when it is removed.

Copy link
Member

Choose a reason for hiding this comment

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

Note that whenever this assertion fails, table-generator would do wrong computations! Actually, it would be to have a test that shows that, can you do that? This would then make it clear that we need a way for setting the correct rounding mode.

benchexec/tablegenerator/conftest.py Outdated Show resolved Hide resolved
test/Dockerfile.python-3.10 Outdated Show resolved Hide resolved
@PhilippWendler
Copy link
Member

Will be superseded by #1043.

@PhilippWendler PhilippWendler deleted the migrate-test-to-pytest branch August 6, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants