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.

Sign up for GitHub

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

ci: Integration Test for all OS's #1537

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
634dbf0
Added matrix oses
Sloox Jan 27, 2021
d46b28d
Windows only
Sloox Jan 28, 2021
7dabb2c
Revert to matrix approach
Sloox Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/full_suite_integration_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@ jobs:
run-it-full-suite:
needs: [ should_run_it ]
if: needs.should_run_it.outputs.run_integration_tests == 'true'
runs-on: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ macos-latest, windows-latest, ubuntu-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

Expand Down