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

feat: add github action #1677

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Conversation

yunnysunny
Copy link
Contributor

@yunnysunny yunnysunny commented Jan 21, 2022

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 :

Oops! Something went wrong! :(

ESLint: 8.6.0

TypeError: Module.createRequire is not a function
    at Object.<anonymous> (/home/runner/work/superagent/superagent/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2359:26)
    at Module._compile (/home/runner/work/superagent/superagent/node_modules/v8-compile-cache/v8-compile-cache.js:192:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (/home/runner/work/superagent/superagent/node_modules/v8-compile-cache/v8-compile-cache.js:159:20)
    at Object.<anonymous> (/home/runner/work/superagent/superagent/node_modules/eslint/lib/cli-engine/cli-engine.js:33:5)
    at Module._compile (/home/runner/work/superagent/superagent/node_modules/v8-compile-cache/v8-compile-cache.js:192:30)

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):

SAUCE_APPIUM_VERSION=1.7 ./node_modules/.bin/zuul -- test/*.js test/client/*.js
(node:2010) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
- testing: chrome @ Mac 11: 97
- testing: firefox @ Mac 10.12: 96
- testing: safari @ Mac 12: 15
- testing: internet explorer @ Windows 2008: 9 10 11
- restarting: <safari 15 on Mac 12>
- restarting: <internet explorer 10 on Windows 2008>
- restarting: <chrome 97 on Mac 11>
- restarting: <firefox 96 on Mac 10.12>
- restarting: <internet explorer 9 on Windows 2008>
- restarting: <internet explorer 10 on Windows 2008>
- restarting: <chrome 97 on Mac 11>
- restarting: <safari 15 on Mac 12>
- restarting: <firefox 96 on Mac 10.12>
- restarting: <internet explorer 9 on Windows 2008>
- restarting: <chrome 97 on Mac 11>
- restarting: <internet explorer 10 on Windows 2008>
- restarting: <chrome 97 on Mac 11>
- restarting: <internet explorer 10 on Windows 2008>
- restarting: <safari 15 on Mac 12>
- restarting: <chrome 97 on Mac 11>
- restarting: <internet explorer 10 on Windows 2008>
- restarting: <firefox 96 on Mac 10.12>
- restarting: <internet explorer 9 on Windows 2008>
- restarting: <safari 15 on Mac 12>
- restarting: <chrome 97 on Mac 11>
- restarting: <internet explorer 10 on Windows 2008>
- restarting: <internet explorer 9 on Windows 2008>
- restarting: <safari 15 on Mac 12>
- failed: <internet explorer 11 on Windows 2008> (0 failed, 0 passed)
- restarting: <internet explorer 9 on Windows 2008>
- failed: <chrome 97 on Mac 11> (0 failed, 0 passed)

/home/runner/work/superagent/superagent/node_modules/zuul/bin/zuul:332
            throw err.message;
            ^
internet explorer@10: localtunnel server returned an error, please try again
(Use `node --trace-uncaught ...` to show where the exception was thrown)

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 :

  1) request
       persistent agent
         should gain a session on POST:
     Uncaught Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_PROTOCOL_ERROR
      at ClientHttp2Stream._destroy (internal/http2/core.js:2212:13)
      at ClientHttp2Stream.destroy (internal/streams/destroy.js:38:8)
      at Http2Stream.onStreamClose (internal/http2/core.js:518:12)

For the problems above, I drop the test of low version nodes, browsers, and http2 in the github action's yml.

@niftylettuce
Copy link
Collaborator

Is this good to merge?

@yunnysunny
Copy link
Contributor Author

yunnysunny commented Jan 23, 2022

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

@yunnysunny yunnysunny mentioned this pull request Jan 23, 2022
@yunnysunny
Copy link
Contributor Author

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.

@titanism titanism merged commit 451cdcd into ladjs:master Mar 29, 2022
@titanism
Copy link
Collaborator

@yunnysunny
Copy link
Contributor Author

yunnysunny commented Mar 29, 2022

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

@titanism
Copy link
Collaborator

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

@seancolyer
Copy link

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

@yunnysunny
Copy link
Contributor Author

yunnysunny commented Apr 2, 2022

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

Has pulled a request (#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.

@yunnysunny
Copy link
Contributor Author

#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 #1716

@titanism
Copy link
Collaborator

@yunnysunny pending results of https://github.com/visionmedia/superagent/actions/runs/2185564459, please follow up if we forget to version bump if successful

@priyanka-kahare-kr
Copy link

Is this package still deprecated ? Looks like this PR has been merged in?

@yunnysunny
Copy link
Contributor Author

@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 #1717 next.

@yunnysunny
Copy link
Contributor Author

The test on Node 10 has been fixed with #1722

@yunnysunny
Copy link
Contributor Author

The test with http2 has been fixed with #1723

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.

5 participants