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

Use binaries for CI testing 😋 #1196

Merged
merged 16 commits into from
Nov 22, 2021
Merged

Use binaries for CI testing 😋 #1196

merged 16 commits into from
Nov 22, 2021

Conversation

zFernand0
Copy link
Member

@zFernand0 zFernand0 commented Nov 4, 2021

Things included here:

  • Ability to run all integrations tests in Daemon mode
  • Ability to run GitHub actions locally using nektos/act

Issues fixed:

Known Test Failures:

@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #1196 (407e844) into next (705c1bf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #1196   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files         356      356           
  Lines        5478     5478           
  Branches      773      773           
=======================================
  Hits         5022     5022           
  Misses        451      451           
  Partials        5        5           

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 06330fa...407e844. Read the comment docs.

@zFernand0 zFernand0 marked this pull request as draft November 4, 2021 22:26
@zFernand0 zFernand0 force-pushed the next-test-with-binary branch 3 times, most recently from 37f82cc to 54d02c5 Compare November 5, 2021 17:31
@zFernand0
Copy link
Member Author

zFernand0 commented Nov 5, 2021

Here is an example of how the matrix will be handled by nektos/act
image

Testing: (notice the colors on the left side) 😋
image

@zFernand0 zFernand0 force-pushed the next-test-with-binary branch from 54d02c5 to a62b570 Compare November 5, 2021 21:06
@zFernand0 zFernand0 force-pushed the next-test-with-binary branch from a62b570 to ec2034b Compare November 5, 2021 21:08
@zFernand0 zFernand0 force-pushed the next-test-with-binary branch from 51356f4 to 361fb8d Compare November 7, 2021 03:50
@zFernand0 zFernand0 marked this pull request as ready for review November 8, 2021 17:41
@zFernand0 zFernand0 requested a review from t1m0thyj November 9, 2021 13:53
@zFernand0
Copy link
Member Author

The zowe-cli.yml workflow may need to be renamed in order for the workflow_dispatch (and default values for its inputs) to take effect

gejohnston
gejohnston previously approved these changes Nov 9, 2021
Copy link
Member

@gejohnston gejohnston left a comment

Choose a reason for hiding this comment

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

Just a couple of questions to complete my understanding. I am NOT requesting any changes.


`0.` The first time it will likely take ~3 minutes to run. Subsequent runs (with `--reuse`) should take less than 2 minutes. <br/>
`1.` Artifact upload and download doesn't work on `nektos/act` (hence why we need to copy the zowe.tgz to a shared Docker volume). <br/>
`2.` `nektos/act` only works with linux distros. <br/>
Copy link
Member

Choose a reason for hiding this comment

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

Nektos/act doc implies that it runs on windows. Is your statement due to the fact that nektos/act only hosts Linux docker images?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, I will rephrase that statement 👍
It should work fine on Windows, Linux, MacOS, and almost everywhere you can have Docker (e.g. Raspberry PI - armv7l) 😋

- name: Setup Binary in PATH
if: github.event.inputs.test-type == "binary"
id: setup-binary
run: tar -xvzf zowe.tgz -C ./node_modules/.bin --overwrite
Copy link
Member

Choose a reason for hiding this comment

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

Should ./node_modules/.bin also be the location that the "zowe config daemon" command puts the zowe executable at a customer site?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think ./node_modules/.bin will work for end users since the where/which zowe location depends on their installation/configuration of node and npm, e.g. using nvm.
For example:

  • /root/.nvm/versions/node/v16.12.0/bin/zowe
  • C:\Users\USER\scoop\apps\nvm\current\nodejs\nodejs\zowe
  • C:\Users\USER\AppData\Roaming\npm\zowe

Maybe something along the lines of "${process.env.ZOWE_CLI_HOME}/_bin" or "~/.zowe/_bin")

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Makes sense. Many customers may not even have the privilege to write to those NodeJS locations when they run the "zowe config daemon" command.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note:
The location where we place the binary changed from ./node_modules/.bin to ./__tests__/__resources__/application_instances becuase of the TestEnvironment.setUp function. (see below)

result.env.PATH = `${nodePath.resolve(__dirname, "../../__resources__/application_instances")}${separator}${process.env.PATH}`;

Base automatically changed from next-zowex-is-zowe to next November 11, 2021 18:54
Signed-off-by: zFernand0 <[email protected]>
@zFernand0
Copy link
Member Author

Even though binary is the default test-type, I believe that if: github.event.inputs.test-type == 'binary' will be skipped until master recognizes the workflow_dispatch event.

test-type:
description: "Specify whether to run tests using the `binary` or regular `nodejs` executable"
default: 'binary'
required: false

@zFernand0
Copy link
Member Author

Note:
The following hack was necessary for the binary to be recognized.
result.env.PATH = `${process.env.PATH}${separator}${nodePath.resolve(__dirname, "../../__resources__/application_instances")}`;

Which means that the setup-binary step will need to be changed back to modifying the path instead of relying on the ./node_modules/.bin location to be recognized by the integration tests

  run: |
    mkdir -p $HOME/.zowe/_bin
    tar -xvzf zowe.tgz -C $HOME/.zowe/_bin
    echo "$HOME/.zowe/_bin" >> $GITHUB_PATH

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

@zFernand0 Thanks for addressing all of my feedback related to the test:act script 🙂

Left a minor comment below, also I'm still having issues with using nektos/act locally 😢 Lots of integration tests fail with an error like the following:

Expected: ""
Received: "/mnt/c/Users/XXX/Projects/zowe-next/zowe-cli/packages/cli/__tests__/zostso/__integration__/issue/__scripts__/issue_help.sh: 2: set: Illegal option -·
"

Based on some Google searching, the error set: Illegal option -· seems to be caused by Bash scripts using Windows line endings (CRLF) instead of Unix ones (LF). I believe what is happening is that on my local machine, the Bash scripts are checked out with CRLF line endings, and then they're getting copied into the Ubuntu Docker container.

Curious what solution you would suggest here - should I change Git settings on my Windows machine to check out files with Unix line endings, or is there an easy way we can convert the line endings when files are copied into the Docker container?

scripts/testCliWorkflow.js Outdated Show resolved Hide resolved
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM! Moved my comment above to #1210 so that it can be addressed separately instead of holding up this PR 🙂

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@t1m0thyj t1m0thyj merged commit 39e4dc8 into next Nov 22, 2021
@t1m0thyj t1m0thyj deleted the next-test-with-binary branch November 22, 2021 22:18
@zFernand0 zFernand0 self-assigned this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants