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

Feature/GitHub action improve #1717

Merged
merged 2 commits into from Apr 26, 2022

Conversation

yunnysunny
Copy link
Contributor

  1. Tigger CI on push and pull request event.
  2. Replace the config of sauce with github action secret enviroment variables.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #1717 (b2b330b) into master (633e467) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1717   +/-   ##
=======================================
  Coverage   86.67%   86.67%           
=======================================
  Files          14       14           
  Lines        1133     1133           
=======================================
  Hits          982      982           
  Misses        151      151           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 633e467...b2b330b. Read the comment docs.


env:
SAUCE_USERNAME: shtylman-superagent
SAUCE_ACCESS_KEY: 39a45464-cb1d-4b8d-aa1f-83c7c04fa673
SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 great! after this is resolved, probably want to rotate these credentials.

@seancolyer
Copy link

I don't know this project well, but I see the timeout bump in #1716. It looks though like this value is overridden in the vast majority of these tests (including the one that fails here if I'm reading correctly). You may need to do something like the following instead:

diff --git a/test/node/query.js b/test/node/query.js
index 2fee688..ae49f39 100644
--- a/test/node/query.js
+++ b/test/node/query.js
@@ -210,5 +210,5 @@ describe('req.query(Object)', () => {
     });

     stream.pipe(request_);
-  });
+  }).timeout(15000);
 });

Again, not super sure of conventions (e.g. repo seems to use a lot of 15_000 vs 15000)

@titanism titanism mentioned this pull request Apr 6, 2022
@yunnysunny
Copy link
Contributor Author

I don't know this project well, but I see the timeout bump in #1716. It looks though like this value is overridden in the vast majority of these tests (including the one that fails here if I'm reading correctly). You may need to do something like the following instead:

diff --git a/test/node/query.js b/test/node/query.js
index 2fee688..ae49f39 100644
--- a/test/node/query.js
+++ b/test/node/query.js
@@ -210,5 +210,5 @@ describe('req.query(Object)', () => {
     });

     stream.pipe(request_);
-  });
+  }).timeout(15000);
 });

Again, not super sure of conventions (e.g. repo seems to use a lot of 15_000 vs 15000)

I has reset the parameter of timeout to 5000, and used timeout function to set pipe testcase separately.

@titanism titanism merged commit bc25a87 into ladjs:master Apr 26, 2022
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

4 participants