Bugfix for path concatenation #56
Conversation
Yeah, we should keep doing it in the foreseeable future. I've been doing it for the bigger change almost every time but didn't repeat it always when you addressed feedback, that's probably how it slipped through. It's annoying but a good motivator to get better test coverage ;) What do you think about a functional end-to-end test where it's just an reproduction run with some trimmed RPKI, IRR and RV data and we just require it to not crash? We wouldn't even need to keep the trimmed input data in the repository, we could have an external repository which the CI job then clones and runs the repo on. That should have caught this, right? I guess this wouldn't even be called an functional test, just a CI task, because it wouldn't require any changes on the test suite I think |
Co-authored-by: Fabian Jahr <[email protected]>
yea was thinking about this and definitely worth doing. I'll set it up 👍 It would have caught this for sure. |
|
doing another run for sanity check |
|
ok successful run based on e85cf4c |
|
tested ACK e85cf4c |
Awesome! Should give us some more peace of mind/early feedback. |
The latest commit on master (using pathlib to do path concatenation in the context) breaks every other path concatenation step in other modules.
I should have done this in one go. Unfortunately, better tests would only have caught some of these issues. The context test is updated here, but we'll need to think about how to test other file handling during the fetching and merge, since file generation and lookup happen outside the context, for example in
collectors/routeviews.py. This PR should cover all the instances of path concatenation for files in the app though.I only caught this by doing a full run, and I'll do that for each change moving forward until we have more tests in place.