-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
Refactor QA setup #393
Refactor QA setup #393
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleksihakli Just two things, we dropped Django 1.11 support in the last release and we are still supporting python 3.5
@nasirhjafri @auvipy @jezdez the The test also fails on PyPy3 in addition to Python 3.6 and should be refactored to account for differing cProfile outputs or disabled altogether if it is not necessary: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT 👍 Thanks @aleksihakli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor updates needed..
project/tests/test_profile_parser.py
Outdated
@unittest.skipIf( | ||
os.environ.get("CONTINUOUS_INTEGRATION") == "true", | ||
"Skip profiling integration test when running CI on due to frequent false negatives" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the test should be written differently then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much agree: however, the test is not simple to fix as the result is a more complex object and ideally the format should be validated, not the contents itself.
Hence, the test would ideally use some sort of a regex matcher for a matching list that is inside the list of profiling results.
This test and the max body size test are also the ones that fail on PyPy3 at the moment.
I would opt a skip in this PR and then a fix in a later PR that updates the test to a more appropriate format, but I am not interested in refactoring the tests at this moment due to time constraints.
@jezdez I believe the version is ready for merging now. Thanks for the accurate review insights! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, some test is failing now, not sure why?
Seems like it's the same test. For whatever reason it isn't skipping the test with the |
Test refactored to test output format instead of specific tool output, seems to be working. Upstreaming the changeset. |
tox
withtox-travis
for more UX friendly test orchestrationpytest
withpytest-django
andpytest-cov
for more readable test outputsetuptools_scm
for inferring package version