Closed yunnysunny closed 2 years ago
Is this good to merge?
Yes. All the unsupported tests mentioned above have been ignored. It will show green status after run tests. We can fix the three problems above some time. We can preview the result of github action in my repository https://github.com/yunnysunny/superagent/actions/runs/1728898488
I think it's important to merge this request, since current project doesn't have an autotest flow. When another new pull request is been merged, we can't distingish whether it's ok.
v7.1.2 released https://github.com/visionmedia/superagent/releases/tag/v7.1.2
I find a error in ci, when test on node 14, a case was failed:
1) req.query(Object)
query-string should be sent on pipe:
Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/superagent/superagent/test/node/query.js)
at listOnTimeout (internal/timers.js:557:17)
at processTimers (internal/timers.js:500:7)
https://github.com/visionmedia/superagent/runs/5730870741?check_suite_focus=true
I re-ran the failed build on node v14 in GitHub CI and it failed again, so I will unpublish in the interim until you confirm it is OK or submit a PR to fix
It would be beneficial to move the SAUCE_USERNAME
and SAUCE_ACCESS_KEY
to use github secrets for the project, then they can be referenced simply as ${{ secrets.SAUCE_ACCESS_KEY}}
in the actions file and not directly be part of source.
Not sure if this had been discussed before, but just noted it while reviewing upgrades 7.1.1 -> 7.1.2
It would be beneficial to move the
SAUCE_USERNAME
andSAUCE_ACCESS_KEY
to use github secrets for the project, then they can be referenced simply as${{ secrets.SAUCE_ACCESS_KEY}}
in the actions file and not directly be part of source.Not sure if this had been discussed before, but just noted it while reviewing upgrades 7.1.1 -> 7.1.2
Has pulled a request (https://github.com/visionmedia/superagent/pull/1717) to resolve it. But the test on browser failed with error message about zuul. I'm not familiar with it, so I just skip the test on browser in CI.
https://github.com/visionmedia/superagent/pull/1716
I re-ran the failed build on node v14 in GitHub CI and it failed again, so I will unpublish in the interim until you confirm it is OK or submit a PR to fix
Try to fix it https://github.com/visionmedia/superagent/pull/1716
@yunnysunny pending results of https://github.com/visionmedia/superagent/actions/runs/2185564459, please follow up if we forget to version bump if successful
Is this package still deprecated ? Looks like this PR has been merged in?
@yunnysunny pending results of https://github.com/visionmedia/superagent/actions/runs/2185564459, please follow up if we forget to version bump if successful
The action has passed. We can merge the request https://github.com/visionmedia/superagent/pull/1717 next.
The test on Node 10 has been fixed with https://github.com/visionmedia/superagent/pull/1722
The test with http2 has been fixed with https://github.com/visionmedia/superagent/pull/1723
I have done some work on bringing github action on current project. In the process of that, I also found some problems.
First, the test on low version node is not supported now (https://github.com/yunnysunny/superagent/runs/4895023400). Since we use a newest version of eslint (8.3.0 in fact), it will emit an error when we run it on node 10.x :
We have to reduce the version of eslint , or skip the lint for node 10 , which needs an refactor of our ci code.
Second, the test of browsers failed with such error (https://github.com/yunnysunny/superagent/runs/4895413544):
I'm not familiar with zuul, and have no idea what happened.
Third, the test on http2 also failed(https://github.com/yunnysunny/superagent/runs/4895023445). It showed many errors , all of them with same reason, this is an example :
For the problems above, I drop the test of low version nodes, browsers, and http2 in the github action's yml.