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

ci: Integration Test for all OS's #1537

Merged
merged 3 commits into from
Jan 28, 2021
Merged

Conversation

Sloox
Copy link
Contributor

@Sloox Sloox commented Jan 27, 2021

Fixes #1527

Test Plan

How do we know the code works?
Need it to be merged before its actual testing

Checklist

  • Integration tests updated

Sorry, something went wrong.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 27, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Sloox
Copy link
Contributor Author

Sloox commented Jan 27, 2021

@flank-it

@github-actions
Copy link
Contributor

Integration tests were triggered at 2021-01-27 14:21:55, you can track progress here

Copy link
Contributor

@piotradamczyk5 piotradamczyk5 left a comment

Choose a reason for hiding this comment

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

it does not work good. IT failed, but no message was posted

@Sloox
Copy link
Contributor Author

Sloox commented Jan 27, 2021

@piotradamczyk5 if i am correct this needs to be merged into master to actually be executable for the IT tests ?

runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ ubuntu-latest, windows-latest, macos-latest ]
outputs:
job_status: ${{ job.status }}
build-scan-url: ${{ steps.run-it.outputs.build-scan-url }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how to handle this build-scan-url, because previously we expect 1 link, but now we will have 3 of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@piotradamczyk5 hmmm, maybe we should chat about it and attempt another solution. I personally feel that the IT should run on all platforms for stability. A requirement of flank is to provide support for all 3 platforms so it would be great.
We can however approach it differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure we could figure out the best solution, we should involve @pawelpasterz because he is a father of integration tests

Copy link
Contributor

Choose a reason for hiding this comment

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

@piotradamczyk5 hmmm, maybe we should chat about it and attempt another solution. I personally feel that the IT should run on all platforms for stability. A requirement of flank is to provide support for all 3 platforms so it would be great.
We can however approach it differently.

IMO that would be great!
Modifying integration processResult script won't be difficult. Question is how to pass 3 build scan URLs?
As I see it there are a couple of action items here:

  1. add more OS ✅
  2. figure out how to pass 3 URLs to the another jobs
  3. change integration processResult to handle 3 URLs (and 3 results actually)
  4. change post-comment_with_results to handle 3 urls

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering how job is behaving with os matrix? Is it run sequentially/in parallel with different os each? How about outputs - are they somehow aggregated? Maybe 2) from my previous comment won't be so difficult. We could set URL, for each os, with suffix, like:

echo "::set-output name=scan-url-${{ matrix.os }}::${{ steps.run-it.outputs.build-scan-url }}"

and then set explicit job output (example):

(...)

outputs:
  build-scan-url-macos: ${{ steps.run-it.outputs.build-scan-url-macos }}
  build-scan-url-ubuntu: ${{ steps.run-it.outputs.build-scan-url-ubuntu }}

(etc...)

Then fetching those data will be easy.
Of course, it's my guess and it should be verified

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, we should decide if we want to do it within this PR scope? IMO enabling new os brings value itself so we can handle additional logic in separated issue/pr.
I think we could create a separate job, running for ubuntu and windows. It will spin in parallel to the existing macOS job and will not interfere with the existing 'processing result logic`. Simultaneously let's create a new issue for updating logic and handle multiple build scan URLs. I would be happy to work on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

captured here: #1539

@pawelpasterz
Copy link
Contributor

@piotradamczyk5 if i am correct this needs to be merged into master to actually be executable for the IT tests ?

AFAIK it is true for newly created workflows. If you just modify an existing one you should be able to launch 'new' version from the branch

@Sloox Sloox requested a review from piotradamczyk5 January 28, 2021 08:35

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Sloox Sloox marked this pull request as draft January 28, 2021 08:39
@Flank Flank deleted a comment from github-actions bot Jan 28, 2021
@Sloox
Copy link
Contributor Author

Sloox commented Jan 28, 2021

@flank-it

@github-actions
Copy link
Contributor

Integration tests were triggered at 2021-01-28 08:48:42, you can track progress here

@Sloox
Copy link
Contributor Author

Sloox commented Jan 28, 2021

@pawelpasterz @piotradamczyk5 as far as i can see it seems like it does need to be merged into master for changes to apply.
The most recent change only runs on windows yet it still has the same OS error as before, "Cannot find windows, linux or mac"
https://github.com/Flank/flank/actions/runs/517401129

@piotradamczyk5
Copy link
Contributor

@piotradamczyk5 if i am correct this needs to be merged into master to actually be executable for the IT tests ?

AFAIK it is true for newly created workflows. If you just modify an existing one you should be able to launch 'new' version from the branch

if you listen for comment trigger, you must merge it first

@pawelpasterz
Copy link
Contributor

@piotradamczyk5 if i am correct this needs to be merged into master to actually be executable for the IT tests ?

AFAIK it is true for newly created workflows. If you just modify an existing one you should be able to launch 'new' version from the branch

if you listen for comment trigger, you must merge it first

Didn't know, thanks!

@Sloox Sloox marked this pull request as ready for review January 28, 2021 10:16
@Sloox
Copy link
Contributor Author

Sloox commented Jan 28, 2021

@piotradamczyk5 @pawelpasterz Shall i try fix the ubuntu workflow apart of this branch before we merge. it appears to be broken again :/

@Sloox Sloox mentioned this pull request Jan 28, 2021
@Sloox Sloox merged commit d0bc98b into master Jan 28, 2021
@Sloox Sloox deleted the #1527-reintroduce-integration-tests branch January 28, 2021 15:50
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-introduce integration tests for windows
4 participants