-
Notifications
You must be signed in to change notification settings - Fork 46
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
editoast: add train_schedule object to search endpoint #9178
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #9178 +/- ##
============================================
+ Coverage 38.45% 38.54% +0.08%
Complexity 2244 2244
============================================
Files 1288 1288
Lines 98429 98454 +25
Branches 3273 3273
============================================
+ Hits 37855 37948 +93
+ Misses 58637 58569 -68
Partials 1937 1937
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8566369
to
6250bbe
Compare
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.
Nice job! Tested and it works great, a few comments on documentation though.
Side note: since the train_name
column isn't indexed to support fuzzy matching, the search
operator is likely much more inefficient than a plain =
.
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.
Thanks for giving an example in the PR description. I'd say it's perfect for actually adding that as a test case of the feature 😈 😜
😭 I'm also unsure if it's necessary to test that feature as it's not meant to change or be broken by anything unless we change the database structure and/or the request result content. |
f9c6093
to
e45c644
Compare
@woshilapin We didn't test the search endpoint because there's a bunch of things to setup and we didn't have the test infrastructure have today. @EthanPERRUZZA for train schedules it should be simpler though, especially since there's no cache. You have in |
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.
LGTM. I agree with @leovalais we can add a test for this search object.
Here an example on how create a train schedule in a test.
Note
For your test, you don't need an infra or a scenario. You just need a timetable and a train schedule.
0ca6f1e
to
73e8366
Compare
e45c644
to
d1d8524
Compare
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.
Looks good, thank you the test. Maybe you can add another simple one to test no results?
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.
Thanks for adding the test! Just a few more details:
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 nitpicks, otherwise LGTM thank you! You have to squash your commits before merging otherwise the CI won't pass.
Signed-off-by: Ethan Perruzza <[email protected]>
0839d57
to
6152bb0
Compare
#9124
Bellow is a test using the below request :
Based on the openapi request : Returns all infra objects of some type according to a hierarchical query.
POST :
{{baseUrl}}/search?page=1&page_size=25
Body :
Answer :