Fix walk board cost comparison and add escalatorReluctance to hash

[optionsome]

Summary

Fixes issue with walkBoardCost equal comparison used in transfer cache and adds escalatorReluctance to hash.

Issue

Closes #5309

Unit tests

Added

Documentation

not needed

Changelog

From title

[optionsome]

This fixes the slowness problem that @vesameskanen had noticed before he went on holiday.

[leonardehrenfried]

Since it's a pretty bad bug, I think you should add a test for it, which will not be hard.

[optionsome]

Since it's a pretty bad bug, I think you should add a test for it, which will not be hard.

Sure, I think I've added tests for this same thing earlier, just have to figure out where.

[leonardehrenfried]

I think adding a few cases in WalkPreferencesTest will be a good idea.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (5ea91e7) 66.23% compared to head (db3dad5) 66.22%.
Report is 2 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5310      +/-   ##
=============================================
- Coverage      66.23%   66.22%   -0.01%     
  Complexity     15129    15129              
=============================================
  Files           1787     1787              
  Lines          69197    69199       +2     
  Branches        7348     7349       +1     
=============================================
- Hits           45832    45827       -5     
- Misses         20881    20891      +10     
+ Partials        2484     2481       -3     

Files Changed	Coverage Δ
...outing/api/request/preference/WalkPreferences.java	85.41% <100.00%> (+0.31%)	⬆️

... and 2 files with indirect coverage changes

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

[optionsome]

I added a test but I really don't like it as it doesn't really prevent someone from accidentally breaking the equals by changing the input/output types. The same problem could occur also if I reuse the static values used to recreate the original method.

[leonardehrenfried]

Suggested change

	// Test that nothing breaks while adding wrapper objects for values
	var copyWithSameValues = subject
	}
	@Test
	void testWithIdenticalUpdatedValues() {
	// Test that nothing breaks while adding wrapper objects for values
	var copyWithSameValues = subject

[leonardehrenfried]

I added a test but I really don't like it as it doesn't really prevent someone from accidentally breaking the equals by changing the input/output types. The same problem could occur also if I reuse the static values used to recreate the original method.

I'm not sure I understand everything completely but could you not test that it is not equals when you pass in other values?

[optionsome]

I'm not sure I understand everything completely but could you not test that it is not equals when you pass in other values?

I've done it now and refactored the tests I had created in last commit so maybe they are less likely to not catch issues as someone is forced to look at the test if input/output types are changed. Still, if new field is added, this doesn't force someone to update the tests automatically.

[leonardehrenfried]

I think if you want to prevent this to happen with future changes then you have to do something clever with reflection in the test.

[vesameskanen]

I can confirm that the problem with slowness got fixed. Itinerary search time in Helsinki region dropped from 15 seconds to 2-3 s.

[optionsome]

I think if you want to prevent this to happen with future changes then you have to do something clever with reflection in the test.

Thought about this but not sure how difficult it would be.

Do you want me to split the test into multiple tests? I would like that too but I thought it would be nice to keep the parameters as close to the test cases as possible so someone doesn't just update them without actually testing the tests that they are used for.

[leonardehrenfried]

Thought about this but not sure how difficult it would be.

I also don't really have an idea. For me personally it would be a nice exercise to write a little utility class that generates the tests for any builder but I will definitely not require it in order to approve.

Do you want me to split the test into multiple tests? I would like that too but I thought it would be nice to keep the parameters as close to the test cases as possible so someone doesn't just update them without actually testing the tests that they are used for.

I have a personal prerference for separate test cases but again, that's not required for my approval.

[leonardehrenfried]

There is an unused method at the bottom of the file. Can you remove it?

[optionsome]

This is ready for review again, I cleaned the tests a bit.