-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Test Fixtures to separate test code from production code #6904
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6904 +/- ##
=============================================
- Coverage 72.10% 72.09% -0.01%
+ Complexity 19663 19658 -5
=============================================
Files 2123 2124 +1
Lines 79428 79509 +81
Branches 8049 8051 +2
=============================================
+ Hits 57269 57324 +55
- Misses 19319 19347 +28
+ Partials 2840 2838 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Just a few minor comments
| FileType type, | ||
| DataSource... children | ||
| ) { | ||
| return new ListCompositeDataSource(name, type, Arrays.asList(children)); |
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.
The ListCompositeDataSource can also be moved here because it is not used in production - and I do not see that it would be relevant for production. If so, we could move it back. Not needed for me to approve PR.
| /** | ||
| * Use this to get a composite data source, bypassing the {@link OtpDataStore}. | ||
| */ | ||
| public static CompositeDataSource compositeSource(File file, FileType type) { |
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 would love to deprecate this to simplify tests, the next method is much easier to use.
|
|
||
| public class GtfsBundleTestFactory { | ||
|
|
||
| public static GtfsBundle forTest(File gtfsFile, @Nullable String feedId) { |
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.
Seems that prettier is not configured to format these files. Can you change that?
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.
Please configure prettier.
|
|
|
I only moved a few classes to demonstrate how it works. I can move more ( |
|
I would be fine with both. It's up to you. |
Summary
This is a suggestion on how we could better separate test code from production code.
Issue
We have a lot of
forTestmethods all over our production code. This could potentially lead to code intended for tests to accidentally be used in production. In my opinion test code and production code should never be mixed.This solution is inspired by the gradle testFixtures plugin. I added a new source to the application module to more or less achieve the same result as the gradle plugin. All classes defined in the new test-fixtures source can be used in all test sources, but not in the production code sources.
To demonstrate the usage I moved the test data factories touched by #6840 into the new test-fixtures source.
Unit tests
no code added -> no tests needed
Documentation
If we decide to move forward with this, it might be a good idea to update the developer guidelines.