-
Notifications
You must be signed in to change notification settings - Fork 821
standardize workflow test pattern #49
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
standardize workflow test pattern #49
Conversation
| uses: actions/checkout@v2 | ||
|
|
||
| - name: Setup Node.js 12.x | ||
| - name: Setup Node.js 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed the empty lines between steps since the file is much larger now (tighten up individual jobs, to make file more skimmable)
| http_proxy: http://localhost:3128 | ||
| https_proxy: http://localhost:3128 | ||
|
|
||
| test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added E2E test job that leverages ./
|
|
||
| - name: Lint | ||
| run: npm run format-check | ||
| test-proxy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched the proxy test to:
- Run in a container, with dns disabled (
options: --dns 127.0.0.). This guarantees that all requests are actually going through the proxy. Previous approach hacked hosts file which does not guarantee all traffic. - Switched to widely used squid container image, rather than maintain our own
- Switched to testing
./during proxy test rather than unit tests
|
|
||
| - name: npm test | ||
| run: npm test | ||
| test-bypass-proxy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an E2E proxy bypass test also
| @@ -0,0 +1,11 @@ | |||
| if (!$args.Count -or !$args[0]) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helper script to validate a specific version of java is in the PATH
209a14f to
60cf21b
Compare
konradpabjan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Really good additions that will help with testing & validation. Testing only one version of Java (the latest, version 13) seems sufficient. No point in kicking off a huge Matrix that is effectively doing the same thing considering there are a lot of versions of Java that we support
+1
FYI, I've been keeping an eye on things externally with a pretty full matrix here: https://github.com/giltene/jdkTestingMatrix to make sure the various versions and spellings (e.g. ea, non-ea). Not sure if we need this to be part of the project or if external monitoring is sufficient, but it does serve as an interesting status if/when people observe problems and want to check to see if it is a systemic issue. |
standardize workflow test pattern
No description provided.