Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Removeroutes-compiler-injected-routes-compilation-with-request-passed scripted test as duplicate#13231

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
ihostage wants to merge1 commit intoplayframework:main
base:main
Choose a base branch
Loading
fromihostage:drop-routes-compiler-injected-routes-compilation-with-request-passed

Conversation

ihostage
Copy link
Member

I thinkroutes-compiler-injected-routes-compilation-with-request-passed already covered byroutes-compiler-routes-compilation androutes-compiler-routes-compilation-java.

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

@mkurz
Copy link
Member

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
MemberAuthor

ihostage commentedJun 9, 2025
edited
Loading

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 onlyone 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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mkurzmkurzAwaiting requested review from mkurz

At least 1 approving review is required to merge this pull request.

Assignees

@mkurzmkurz

Labels
None yet
Projects
Status: 👀 In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@ihostage@mkurz

[8]ページ先頭

©2009-2025 Movatter.jp