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

Introduce builder for RealtimeTestEnvironment #6043

Open
wants to merge 26 commits into
base: dev-2.x
Choose a base branch
from

Conversation

leonardehrenfried
Copy link
Member

Summary

In a previous PR we said that we want to be able to configure the RealtimeTestEnvironment instances and only add those trips that we need. This PR refactors the environment and adds a builder that allows you do to exactly.

This is made possible by extracting all constants into a separate class that is used throughout the test code.

Issue

#4816

Unit tests

This is just tests.

@leonardehrenfried leonardehrenfried added technical debt RealTimeUpdate The issue/PR is related to RealTime updates Skip Changelog labels Sep 4, 2024
@leonardehrenfried leonardehrenfried requested a review from a team as a code owner September 4, 2024 10:39
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.78%. Comparing base (c00ea1e) to head (b479e20).

Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #6043   +/-   ##
==========================================
  Coverage      69.78%   69.78%           
  Complexity     17356    17356           
==========================================
  Files           1962     1962           
  Lines          74359    74359           
  Branches        7624     7624           
==========================================
+ Hits           51892    51893    +1     
+ Misses         19823    19821    -2     
- Partials        2644     2645    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice with a builder for the environments!


var tripId = env.trip2.getId().getId();
var tripId = TRIP_2_ID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably get rid of this tripId variable now that it doesn't reallly improve readability anymore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworked the handling of the IDs here: c97d90a

@optionsome optionsome marked this pull request as draft September 10, 2024 09:53
@leonardehrenfried leonardehrenfried marked this pull request as ready for review September 10, 2024 15:10
@leonardehrenfried
Copy link
Member Author

I have now extracted builders not only for the environment but also for the trips that are in it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good to me. Just a comment on a name

@t2gran t2gran added this to the 2.7 (next release) milestone Sep 18, 2024
@leonardehrenfried leonardehrenfried requested review from jtorin and removed request for t2gran September 20, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RealTimeUpdate The issue/PR is related to RealTime updates Skip Changelog technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants