Add missing vehicle to passenger log requirements

[syhwawa]

Fix missing vehicles error for passenger_log in the event handler when Elara fails to output the passenger log. Original PR #229

[Theodore-Chatziioannou]

Good catch

[KasiaKoz]

It would be good to see a test added that reproduces the error before it is fixed, and passing after the fix. I have a feeling this got missed out before (in this PR: #229) because there isn't much test coverage in this spot, so adding just that one extra test might help already with catching problems in the future.

[fredshone]

nice catch @syhwawa - I'm not too bothered about a test - although kasia is right - we are also expecting to freeze dev on elara in any case

[fredshone]

not sure what to make of this new test. Asserts something that can only ever be true, ie doesn't actually check behaviour.

IF you really want to test then the existing pattern in elara is to create the handler (eg here) (but note this requires using a bunch of fixtures) AND then test using the handler to process events (eg) AND then finalise.

You would need to add all these for the handler. Bit of a pain.

OR there is a cheaty way here that tries to "load all tools". Maybe check why this not working.

[syhwawa]

not sure what to make of this new test. Asserts something that can only ever be true, ie doesn't actually check behaviour.

IF you really want to test then the existing pattern in elara is to create the handler (eg here) (but note this requires using a bunch of fixtures) AND then test using the handler to process events (eg) AND then finalise.

You would need to add all these for the handler. Bit of a pain.

OR there is a cheaty way here that tries to "load all tools". Maybe check why this not working.

Thanks for the comment and suggestion @fredshone.

My initial idea is not acuatlly testing the handler in this unit test. I am wondering just to assert list == list for the requirements List from the each Class which having various inputs in the event_handler.py, to check if they match the added requirments list in this unit test. so it will pop up an error like below when we miss to add 'vehicles' in the requirments for VehiclePassengerLog Class when we add a new input for various Class.

./tests/test_2_event_handlers.py::test_class_requirements Failed: [undefined]AssertionError: Requirements do not match for class VehiclePassengerLog
assert ['events', 'transit_schedule'] == ['events', 'transit_schedule', 'vehicles']
  Right contains one more item: 'vehicles'
  Use -v to get more diff
test_config = <elara.config.Config object at 0x7fa245e46df0>

But I do forget the VehiclePassengerLog Class in the test again which a bit of embarrassing 🐼

[fredshone]

I understand, but think that the this is not really a useful test because whatever shenanigans someone get's up to with the handlers in future - this test will never fail. Basically we are setting a = b in the module then testing a = b in the test. This can only fail if (i) we add some methods that mess with the requirments (not likely) or (ii) python breaks.

But if you insists i am happy for you to merge (I cannot swing so wildly from "don't need tests" to "that is not the correct test" :))

[syhwawa]

Thanks for the suggestions. @fredshone You're right, I see the examples tests you've provided make sense.

Considering the time and effort required for implementing these tests. I am a bit ashamed to decide to not extend the test coverage in this PR 😞.

Indeed, it needs the more extensive checks that you've pointed out, and maybe plan to revisit this as a part of a future task dedicated specifically to enhancing the test or move to develop a light post-process tool 🚀

[fredshone]

[like] Fred Shone reacted to your message:

…

________________________________ From: Yuhao Sun ***@***.***> Sent: Tuesday, May 23, 2023 3:37:01 PM To: arup-group/elara ***@***.***> Cc: Fred Shone ***@***.***>; Mention ***@***.***> Subject: Re: [arup-group/elara] Add missing vehicle to passenger log requirements (PR #230) Thanks for the suggestions. @fredshone<https://github.com/fredshone> You're right, I see the examples tests you've provided make sense. Considering the time and effort required for implementing these tests. I am a bit ashamed to decide to not extend the test coverage in this PR 😞. Indeed, it needs the more extensive checks that you've pointed out, and maybe plan to revisit this as a part of a future task dedicated specifically to enhancing the test or move to develop a light post-process tool 🚀 — Reply to this email directly, view it on GitHub<#230 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGJJMPLLILJY2PQ6DSHIAZ3XHTKR3ANCNFSM6AAAAAAYE56IQY>. You are receiving this because you were mentioned.Message ID: ***@***.***> ____________________________________________________________ Electronic mail messages entering and leaving Arup business systems are scanned for viruses and acceptability of content.