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

Add aiohttp regression tests to CI #250

Merged
merged 17 commits into from
Oct 3, 2023
Merged

Add aiohttp regression tests to CI #250

merged 17 commits into from
Oct 3, 2023

Conversation

Dreamsorcerer
Copy link
Contributor

No description provided.

@Dreamsorcerer
Copy link
Contributor Author

I'm testing within the fork. It almost works, but fails to import the compiled parser for some reason. Will get back when it's fixed.

@Dreamsorcerer Dreamsorcerer marked this pull request as ready for review September 18, 2023 16:24
@Dreamsorcerer
Copy link
Contributor Author

Dreamsorcerer commented Sep 18, 2023

@ShogunPanda OK, that's ready to go and can be merged in now.

The 100_continue test failures are #249. The other test failure is just due to the wording of an error message being changed. I'll update that final test when we have a new release that fixes the other 2 tests.

@ShogunPanda
Copy link
Contributor

@Dreamsorcerer This looks go to me. Once aiohttp is all green with llhttp I'll merge this.

@Dreamsorcerer
Copy link
Contributor Author

Well, if you merge it first, then you should see the tests pass once #249 is fixed. We need the regression resolved before we can push a new release.

@benz0li
Copy link

benz0li commented Oct 3, 2023

Once aiohttp is all green with llhttp I'll merge this.

@ShogunPanda The issue is on llhttp's side. If you do not agree, express your opinion accordingly.

@ShogunPanda
Copy link
Contributor

@benz0li @Dreamsorcerer I get a failure on llhttp CI. Can you please verify it?

@benz0li
Copy link

benz0li commented Oct 3, 2023

@benz0li @Dreamsorcerer I get a failure on llhttp CI. Can you please verify it?

@ShogunPanda If you are talking about https://github.com/nodejs/llhttp/actions/runs/6225384656/job/16918217042?pr=250:

The 100_continue test failures are #249.

This needs to be fixed on llhttp's side.

@benz0li
Copy link

benz0li commented Oct 3, 2023

@Dreamsorcerer @ShogunPanda Apparently you both disagree about which side is causing the problem.

Please clarify this as soon as possible. Thank you.

@Dreamsorcerer
Copy link
Contributor Author

Yes, as mentioned above, the 100_continue failures:
https://github.com/nodejs/llhttp/actions/runs/6225384656/job/16918217042?pr=250#step:9:701

They are caused by #249, so if you merge this in now, you can use the CI to verify any fixes to that issue (i.e. you'll see those 2 failures disappear in the PR that fixes it)..

@benz0li
Copy link

benz0li commented Oct 3, 2023

@indutny As the initiator of llhttp, I would greatly appreciate your objective view on this.

@ShogunPanda ShogunPanda merged commit 7087d5d into nodejs:main Oct 3, 2023
7 of 8 checks passed
@ShogunPanda
Copy link
Contributor

Released v9.1.3.

@Dreamsorcerer
Copy link
Contributor Author

@ShogunPanda Thanks a lot. That's merged now, so if you rerun the last test run on master, then it should go green now:
https://github.com/nodejs/llhttp/actions/runs/6394836947/job/17357138399

@Dreamsorcerer Dreamsorcerer deleted the patch-1 branch October 3, 2023 16:48
@benz0li benz0li mentioned this pull request Oct 4, 2023
1 task
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.

None yet

3 participants