Skip to content

Conversation

@ihostage
Copy link
Member

@ihostage ihostage commented Apr 4, 2025

I think routes-compiler-injected-routes-compilation-with-request-passed already covered by routes-compiler-routes-compilation and routes-compiler-routes-compilation-java.

I added a few cases related the name, type and position of Request param into routes-compiler-routes-compilation-java. Therefore, I think we don't need this duplicated test now.

@mkurz
Copy link
Member

mkurz commented Jun 6, 2025

I was taking a quick look at this PR and also #13230. It's a bit much to analyse.
I am scared we remove some test(s) which covers some edge case. I know the tests are often similiar and some or many are duplicates. But I also remember that I did every now and then add just one test to on of those scripted tests, and again I am scared we do not take care enough and now would just remove such a test.
Because it's hard to analyze currently and don't want to spend the time for this maybe we should just keep te tests? I mean do we really save so much? I know takes a couple of minutes to run, but I will not remove them until I am 100% sure that it's ok the remove them...

@ihostage
Copy link
Member Author

ihostage commented Jun 9, 2025

I am scared we remove some test(s) which covers some edge case.

Yes, Matthias, you are absolutely right in common. I also was scared.

I know takes a couple of minutes to run, but I will not remove them until I am 100% sure that it's ok the remove them...

When I was working on this PR I was thinking about how to simplify our codebase. As you said a few corner cases were implemented only in one test and this made the cost of supporting these tests very expensive. Spending time on CI wasn't a priority for me, I wanted only one example (actually, two java/scala 😄) that covered all cases related to route compilation to simplify maintenance it in future.

I was taking a quick look at this PR and also #13230. It's a bit much to analyse.
...
Because it's hard to analyze currently and don't want to spend the time for this ...

But I already spent this time a made this analysis 😂 Found a few missed cases and added them in these PRs and removed all other cases as duplicated.

@ihostage ihostage force-pushed the drop-routes-compiler-injected-routes-compilation-with-request-passed branch from 56d7a18 to 9f423a2 Compare October 20, 2025 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

2 participants