Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite WebRequest.getParameters (#844) #851

Merged

Conversation

Crydust
Copy link
Contributor

@Crydust Crydust commented Aug 14, 2024

Each time I tested the implementation I found new things that went wrong.
Now I've created a parametrized test to cover all cases except method == TRACE && encoding.equals("text/plain"), which behaves oddly.
It takes 40 minutes to run with firefox, so I stored the output in a xml file as "golden master".
Then I ran the same tests with htmlunit until the ouputs were the same
This new implementation should be as correct as it'll get.
All my tests (bothh old and new) are now passing.

See
https://github.com/Crydust/testcasehtmlunit
https://github.com/Crydust/testcasehtmlunit2

@Crydust Crydust force-pushed the kristof/include-query-in-getParameters branch from a2526dc to c042de4 Compare August 14, 2024 18:29
@rbri
Copy link
Member

rbri commented Aug 14, 2024

Great, will try to ontegrate this in the HtmlUnit ci tests

@Crydust
Copy link
Contributor Author

Crydust commented Aug 14, 2024

I’m not sure the 3000+ tests should all be included in a test suite that is regularly run. It is time consuming. What did worry me though is that I changed the getParameters substantially without breaking any tests. So maybe one or two added unit tests might be useful.

@Crydust
Copy link
Contributor Author

Crydust commented Aug 29, 2024

Should I assist with integrating a unit test into ci?

@rbri rbri merged commit 2cb407c into HtmlUnit:master Sep 5, 2024
7 checks passed
@rbri
Copy link
Member

rbri commented Sep 5, 2024

ok, have merged this.

My dream for a test case

  • have a separate repo for the spring compatibility test
  • have a separate ci run on the jenkins (like for the other huge stuff)
  • inside the repo have different projects/structures for the different spring version
  • being able to run the whole test suite with two different setups
    • run with real spring to adjust the expectations if a new spring version is out
    • the default run with htmlunit to make sure we do not break this

It will be fantastic if you can prepare something in that way.

@rbri
Copy link
Member

rbri commented Sep 5, 2024

Have merged your stuff and will make a snapshot build out of it if not too many tests are failing

Sorry for being not that responsive during the last days - was on a workshop and will be out for two more weeks to have a holiday break. Afterwards we can work together to get this finally done.

@rbri
Copy link
Member

rbri commented Sep 5, 2024

after applying this i had to ignore the org.htmlunit.WebRequest2Test suite for the moment

maybe the test is later replaced by the new project

@rbri
Copy link
Member

rbri commented Sep 5, 2024

snapshot updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants