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

Testing Module #28000

Merged
merged 8 commits into from
Oct 7, 2024
Merged

Testing Module #28000

merged 8 commits into from
Oct 7, 2024

Conversation

blaggacao
Copy link
Collaborator

@blaggacao blaggacao commented Oct 6, 2024

CI is breaking? Try OLD_FRAPPE_TEST_CLASS_RECORDS_PRELOAD=1 which brings back the preloading behaviour for old FrappeTestCase classes.
A not-really-public API within test_runner.py has mainly been removed with this PR. If you have been bitten and want it temporarily back, please comment to this PR, I'll add it back to the dumpster. I'll try hard to keep an SLA of 2 days during the first 2 weeks after merge, including weekends, best-effort afterwards.

https://github.com/frappe/frappe/wiki#testing-guide

Enhance Frappe Testing Module Documentation

This PR improves the documentation and structure of the Frappe testing module:

  • Add explanatory docstrings to key files (new: runner.py, discovery.py, environment.py, result.py, init.py)
  • Create a README.md for the frappe/testing module, providing an overview and directing users to the well-commented code
  • Improve code organization and readability
  • Enhance logging configuration in init.py
  • Ends up fixiing some regressions of previous PRs in the series (mainly: Testing Improvements 3 #27995 & bis)

These changes aim to make the testing module more accessible and easier to understand for developers working with the Frappe framework.

@blaggacao blaggacao marked this pull request as draft October 6, 2024 20:38
@blaggacao blaggacao self-assigned this Oct 6, 2024
@blaggacao blaggacao force-pushed the testing branch 2 times, most recently from 7f55bbd to 47f7770 Compare October 6, 2024 23:28
@blaggacao blaggacao marked this pull request as ready for review October 6, 2024 23:38
@blaggacao blaggacao merged commit 7e453ea into frappe:develop Oct 7, 2024
24 checks passed
@blaggacao blaggacao deleted the testing branch October 7, 2024 07:46
@rohitwaghchaure

This comment was marked as resolved.

@blaggacao

This comment was marked as resolved.

@ljain112

This comment was marked as resolved.

@blaggacao

This comment was marked as resolved.

@Abdeali099

This comment was marked as resolved.

@blaggacao

This comment was marked as resolved.

@harshtandiya
Copy link
Contributor

Earlier, whenever tests were enabled, testusers were created with specific Roles and User Types. Was that removed in this?

@blaggacao
Copy link
Collaborator Author

blaggacao commented Oct 10, 2024

@harshtandiya To exactly understand when test records are created, just a couple of minutes back, I implemented increased logging (alongside some fixes) to the new ./logs/frappe.testing.log.

Would you mind updating to or beyond #28071 and sharing that logfile here? Don't forget to clear the previous logfile to not get confused with the different runs.

What should happen:

  • All doctype creations should be logged there
  • When in the sequence of events they are created should be logged there, too

That would allow me to better understand what's going on and to pinpoint how your expectations are missed. It might also be that one of the fixes would already take effect and the old behavior is restored, though.

Finally, #28070 changed the logfile format, so it might be necessary to reinstall the test site to clear the log and database and get them back in sync.

@Ninad1306
Copy link
Contributor

Ninad1306 commented Oct 11, 2024

Skip test records may still be important ??

  • skip_test_records is not just used to delay creating test records until needed.
  • For example, ERPNext generates fiscal year test records starting in January, but India Compliance requires the fiscal year to start in April.
  • In this case, skip_test_records is used to ensure that India Compliance tests use their own test records, avoiding conflicts.

@blaggacao can you look into this ??

@blaggacao

This comment has been minimized.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Oct 12, 2024

@Ninad1306 I have a little update for you which I hope will be useful.

After #28080 ERPNext is now fully compliant with lazy loading (at least on a global test run), also via run-prarallel-tests. This is why the CI ouput now can be easily inspected for the test records created and when they are first (!) required and created:

image

You'd now only have to check for the test records that offend your tests and ensure your test runs prior to that other test.

Alternatively, you can create a context manager for your test, which temporarily patches the given database values of the offending global test records and restores them afterwards. You can add any context manager easily via cls.enterClassContext(my_cm()) or self.enterContext(my_cm()). They will automatically exit when the test or class is teared down regardless of success or failure. In this way, you can execute your test under conditions of a tightly controlled global state and you can avoid resorting to any CLI-based work-around, entirely.

@blaggacao

This comment has been minimized.

@blaggacao

This comment has been minimized.

@blaggacao

This comment has been minimized.

@blaggacao

This comment has been minimized.

@ruthra-kumar
Copy link
Member

ruthra-kumar commented Oct 14, 2024

@blaggacao

Added #28000 (comment) and #28000 (comment) as separate guides in Wiki as requested https://github.com/frappe/frappe/wiki#testing-guide

@blaggacao
Copy link
Collaborator Author

@gavindsouza
Copy link
Collaborator

Why is my CI breaking? Is there a way to keep using the old test runner? I was content with the test suites for my apps and now updating the application(s) has caused a lot of breakages. I recognize there's a shift in the design principles of the new testing module, but I don't want to arbitrarily take on this overhead now (The lazy loading of the records is one of the breaking changes I can point out towards).

2 days in, and now it's clear to me that fixing the CI/test runners can't be treated as an ad-hoc task. The lack of clarity of the implications/changes in the PR descriptions and commit messages are of no help. I've gone through many of your PRs and cannot track the changes very effectively. Maintaining a single thread/issue with all testing module changes would've gone a long way.

Bug: It also seems like, despite the "deprecation" of the FrappeTestCase, the test runner doesn't actually pick all the relevant cases, eg: I have 30 tests and only 5 run - thankfully I track coverage so that was a sufficient indicator of this behaviour.

@blaggacao

This comment was marked as outdated.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Nov 4, 2024

Bug: It also seems like, despite the "deprecation" of the FrappeTestCase, the test runner doesn't actually pick all the relevant cases, eg: I have 30 tests and only 5 run - thankfully I track coverage so that was a sufficient indicator of this behaviour.

That seems unlikely, at first sight, because discovery essentially only wraps unittest upstream logic.

I think I have a reproducer: The-Commit-Company/raven#1094 (comment)

I'll dig on the basis of that reference.


@gavindsouza This fix should fully cover your damage (except valuable time spent, sincere apologies for that) and you should be able to ignore any and everything about the recent refactoring in the testing framework until a point in time of your own choosing. The expectation to eventually move to a more granular, conscientious, well-specced and (in better ways) quality-supporting test framework is, as of now, deferred to v17 (i.e. in more than a year) and there may also be more improvements incoming, still. So it makes absolutely sense to defer migration, although I'm grateful for feedback and improvement suggestions.

Let me be clear: the backwards compatibility should have been maintained at all times, and I really apologize for not having caught that properly in the turbulence of figuring out and iterating in contrived ways on a path towards overall better testing practices.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Nov 13, 2024

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants