kolibri icon indicating copy to clipboard operation
kolibri copied to clipboard

Pytest upgrade 3.7.1 -> 6.2.5

Open thesujai opened this issue 2 years ago • 16 comments

Summary

When i run test on files individually all are passing, but while running them at once it fails

References

Fixes #11728 …

Reviewer guidance


Testing checklist

  • [ ] Contributor has fully tested the PR manually
  • [ ] If there are any front-end changes, before/after screenshots are included
  • [ ] Critical user journeys are covered by Gherkin stories
  • [ ] Critical and brittle code paths are covered by unit tests

PR process

  • [ ] PR has the correct target branch and milestone
  • [x] PR has 'needs review' or 'work-in-progress' label
  • [ ] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
  • [ ] If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

thesujai avatar Jan 20 '24 13:01 thesujai

This work should be targeted to develop and targeted there, as this version of pytest is not compatible with Python 2.7, which has been dropped in develop.

rtibbles avatar Jan 20 '24 16:01 rtibbles

@rtibbles okay i will change the target in final pr, but i am facing this issue In develop branch only, after changing versions in requirements Check that test_utils is failing when i ran all at once Screenshot from 2024-01-20 22-11-01 But it passes when i run this individually Screenshot from 2024-01-20 22-11-17

thesujai avatar Jan 20 '24 16:01 thesujai

Interesting - there must be something else at play here. One possibility is a change in pytest that is causing state to be persisted between tests.

Seeing the traceback from the failed tests would help diagnose further!

Also it may be worth seeing if any of the other pytest plugins could be upgraded.

rtibbles avatar Jan 20 '24 18:01 rtibbles

@rtibbles After some debugging, what i can observe is if exclude all test from test_dbrestore.py (for testing only)then all my tests are passing, except for one: Screenshot from 2024-01-22 19-36-17 This test was passing fine when running standalone.

Further more, i just configured the pytest.ini to run only test_content_sync_hook.py and utils/test_content_request.py, and it failed giving the exact above error. So I think this test_content_sync_hook.py is causing some state to be persisted here.

let me know what can be done further

thesujai avatar Jan 22 '24 14:01 thesujai

That's some good debugging - I notice that the test_content_sync_hook test case inherits from the test_content_request testcase: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/test/test_content_sync_hook.py#L12

So it may be that this is the cause of the interaction? Looking more closely at the IncompleteDownloadRequest test case may shed some more light here.

What errors are you seeing from the test_dbrestore test suite?

rtibbles avatar Jan 27 '24 00:01 rtibbles

While running test_dbrestore standalone I am getting this: Screenshot from 2024-02-01 11-02-51

Also one more behaviour that test are getting collected with pytest6.2.5 and pytest-django 3.6.0 With pytest3.7.1 and pytest-django3.3.3:(running deviceadmin): Screenshot from 2024-02-01 11-07-07

With pytest6.2.5 and pytest-django3.6.0:(running deviceadmin): Screenshot from 2024-02-01 11-10-40 Errors are: Screenshot from 2024-02-01 11-10-54

As much as i know test are failing because of pytest-django upgrade to 3.6.0 as the pytest.config is changed to request.config and high version of pytest uses that

thesujai avatar Feb 01 '24 05:02 thesujai

Sorry @thesujai I missed the follow up comments here as I was out of the office. It looks like the dbrestore tests might be fixable by tweaking the configuration for pytest-django?

For the other test where the bare assertion is failing, can you look at where the assertion is failing and see what it is meant to be asserting?

rtibbles avatar Feb 13 '24 21:02 rtibbles

Once we have merged this PR (which upgrades Django to 3.2), it would be good to rebase this PR onto develop, as that may help with some of the errors.

rtibbles avatar Mar 09 '24 22:03 rtibbles

I have rebased this PR onto latest develop and pushed, let's see how the tests shake out now!

rtibbles avatar May 07 '24 00:05 rtibbles

I'm not sure if it helped still getting 657 failing tests with 530 errors. I cannot even get what is causing the tests to fail because running most of them individually is okay but running them together is not so okay. As the log file I generated(by pytest) is 212873 lines long, so not able to read it either.

One idea I can think of is having a global teardown code that will reset all the states after each test, making tests stateless. Ref: https://stackoverflow.com/questions/22627659/run-code-before-and-after-each-test-in-py-test

thesujai avatar May 07 '24 09:05 thesujai

No - it doesn't seem to have helped. Although, you had mentioned above that there was some configuration change needed for the upgraded pytest-django plugin?

rtibbles avatar May 07 '24 17:05 rtibbles

Sorry for missing your previous comment on this, I missed it. I was not talking about configuration change needed for the upgraded pytest-django plugin

I was referring to this changelog https://pytest-django.readthedocs.io/en/latest/changelog.html#id35 and https://docs.pytest.org/en/8.2.x/changelog.html#removals

I trying to say that changelog is the only reason for upgrading pytest-django as the older version of pytest-django is not compatible with the new version pytest(due to that changelog only)

thesujai avatar May 07 '24 18:05 thesujai

In Hiatus until @rtibbles has the bandwidth to pick it up!

akolson avatar May 09 '24 15:05 akolson

@rtibbles I think we should move this from the hiatus status, after 2.7 support has been finished, and specially if new versions of django are been used (as a reference, KDP has been using 6.2.1 since September 2022)

jredrejo avatar Jun 17 '24 17:06 jredrejo

Yes, I have the overall issue for upgrading to support Python 3.12, which this is a prerequisite for.

rtibbles avatar Jun 18 '24 16:06 rtibbles