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

Breakout unit testing per OS. Issue: #353 #354

Merged
merged 13 commits into from
Feb 3, 2022
Merged

Breakout unit testing per OS. Issue: #353 #354

merged 13 commits into from
Feb 3, 2022

Conversation

gbchd
Copy link
Contributor

@gbchd gbchd commented Feb 2, 2022

Description of the issue

The "build.yml" workflow does both linux and macos build and test. This could lead to the failure of both of them if one of them fails and then require to rerun everything all over again for both OS, which is both costly and time consuming.
Link to the issue: #353

Description of changes

Split the "build.yml" workflow into "build-test-linux.yml" and "build-test-macos.yml".

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Pushed the same code in another fork and have run both of these workflow successfully .

@@ -1,4 +1,4 @@
name: build-and-test
name: build-and-test-macos
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need one workflow that uploads test coverage. Can you remove the upload step from this workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be ok now

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can remember, we do need coverage uploads for each OS because some tests don't run on every runner

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no conflict/overriding that happens when we have multiple coverages being published for the same PR? I'll have to look into that again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Read up on Codecov and there's no issue: https://docs.codecov.com/docs/merging-reports We should upload coverage from all of the workflows

@khanhntd
Copy link
Contributor

khanhntd commented Feb 2, 2022

Hi @GuillaumeBchd , thanks for opening up a PR for this. Still did not expect this though after opening my PR #355. However, there are some changes but you can look at my changes and will answer all of the changes though


- name: Test
run: make test

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the

      - name: Upload coverage to Codecov
        uses: codecov/codecov-action@v2
        with:
          verbose: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to have all of the runners publish test coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming we retrying a single workflow and due to facts that our test are flakiness, so sometimes we cannot cover all of our tests. Therefore, uploading a test coverage for single workflow is to address this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uploading coverage from all of the OS workflows does not address test flakiness. That being said, it does make sense that we need to run it on all our OS because windows tests would only get run on the windows workflow, and Codecov does automatic report merging, so there's no conflict there. I was under the impression that different code coverage reports would overwrite previous ones on the same commit.

@khanhntd
Copy link
Contributor

khanhntd commented Feb 2, 2022

Please also changes to make test after using choco install make and make build for the window pipeline https://github.com/aws/amazon-cloudwatch-agent/blob/master/.github/workflows/build-test-windows.yml#L45

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #354 (f085214) into master (8443773) will decrease coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
- Coverage   57.33%   57.15%   -0.18%     
==========================================
  Files         359      370      +11     
  Lines       16471    17231     +760     
==========================================
+ Hits         9443     9848     +405     
- Misses       6488     6805     +317     
- Partials      540      578      +38     
Impacted Files Coverage Δ
...gins/inputs/win_perf_counters/win_perf_counters.go 68.15% <0.00%> (ø)
...gins/inputs/windows_event_log/windows_event_log.go 26.08% <0.00%> (ø)
plugins/inputs/win_perf_counters/pdh.go 65.47% <0.00%> (ø)
translator/util/platform_windows.go 75.00% <0.00%> (ø)
...windows_event_log/wineventlog/wineventlogrecord.go 59.09% <0.00%> (ø)
...s/inputs/windows_event_log/wineventlog/sys_call.go 47.36% <0.00%> (ø)
...nputs/windows_event_log/wineventlog/wineventlog.go 36.32% <0.00%> (ø)
plugins/inputs/logfile/tail/tail_windows.go 100.00% <0.00%> (ø)
...puts/logfile/tail/file_deleting_checker_windows.go 36.36% <0.00%> (ø)
...gins/inputs/windows_event_log/wineventlog/utils.go 61.53% <0.00%> (ø)
... and 4 more

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 8443773...f085214. Read the comment docs.

@khanhntd
Copy link
Contributor

khanhntd commented Feb 2, 2022

Please delete build.yaml too if we implement these changes.

@gbchd
Copy link
Contributor Author

gbchd commented Feb 2, 2022

Please delete build.yaml too if we implement these changes.

It should already be deleted

@khanhntd
Copy link
Contributor

khanhntd commented Feb 2, 2022

Please delete build.yaml too if we implement these changes.

It should already be deleted

You are right. My mistake though.

Copy link
Contributor

@khanhntd khanhntd left a comment

Choose a reason for hiding this comment

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

LGTM.

@SaxyPandaBear SaxyPandaBear merged commit 8f5c55d into aws:master Feb 3, 2022
sethAmazon pushed a commit to sethAmazon/amazon-cloudwatch-agent that referenced this pull request Aug 21, 2023
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.

5 participants