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

AAP-38528 Make default state passing for coverage targets #15772

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

AlanCoding
Copy link
Member

SUMMARY

Reference:

#15753

That PR literally moves test files and it fails checks

Screenshot from 2025-01-23 21-20-49

And it seems to be some dissonance between present coverage values, and the coverage of the diff. I'm not sure if we should have a metric for coverage in the first place, but I digress.

For merges

Screenshot from 2025-01-23 21-19-52

This adjusts 3 values, corresponding to those 3 ❌ s so that we may be X-free for the happy path. Hopefully. Soon.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

@AlanCoding AlanCoding changed the title Make default state passing for coverage targets AAP-38528 Make default state passing for coverage targets Jan 24, 2025
@AlanCoding
Copy link
Member Author

Well now it's showing this.

codecov/patch/typing — Coverage not affected when comparing 534c312...8014138

I'm honestly happy with this as-is. If people have more ideas, I'm good to go with those, but for the near-term I think this helps.

@webknjaz
Copy link
Member

@AlanCoding sorry, I just noticed the notification. Will respond shortly.

@webknjaz
Copy link
Member

@AlanCoding @PabloHiro I've posted a comment with the entire context that I had in my head (unless I've forgotten some important nuance, it should be complete). Hopefully, this will help you reevaluate the action items.

In general, I think that the procedure of reacting to Codecov reports needs to be made known, plus a few tweaks in two configs may be needed.

@PabloHiro PabloHiro requested review from Jaapis and adrisala January 29, 2025 09:40
@AlanCoding
Copy link
Member Author

With my recent merge, test coverage is >95%, so I've undone that change @webknjaz , but I still don't have actual coverage data for tests, which would be actionable.

I've implemented a form of what @PabloHiro suggests, basically not change the patch criteria but address the project coverage threshold.

But again, none of this is truly useful, because integration tests report no coverage. That would be technically viable but it not on our priorities.

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.25%. Comparing base (c418bc0) to head (9c3696a).
Report is 2 commits behind head on devel.

✅ All tests successful. No failed tests found.

@AlanCoding AlanCoding merged commit 30b0c19 into ansible:devel Jan 31, 2025
27 checks passed
@webknjaz
Copy link
Member

because integration tests report no coverage.

@AlanCoding what do you mean? Some collection tests? There's integration tests from the collection with the integration flag that are already being reported: https://app.codecov.io/gh/ansible/awx?flags[]=integration. Do you see any files there that are missing?

If I can just go to SonarCloud and view all uncovered lines in tests, then great.

You can go to Codecov and it'll show the uncovered lines: https://app.codecov.io/gh/ansible/awx/tree/devel/awx%2Fmain%2Ftests?flags%5B0%5D=pytest.

@AlanCoding
Copy link
Member Author

@webknjaz Help me file an issue for this. And let me know if I'm wrong on anything -

The collection integration tests gather coverage for the client interacting with the API. It does not gather coverage data on what happens in the server.

https://github.com/ansible/awx/blob/devel/.github/actions/run_awx_devel/action.yml

Both the collection integration tests and the awx/main/tests/live tests used the shared run_awx_devel action. This starts docker-compose which runs in the background while a pytest process runs tests. The explicit goal of the live tests are to take actions will then trigger an action in background services, and then test an outcome.

Presumably, we could set up coverage data gathering when the server starts and combine this later. Without that, I can't do a whole lot with this coverage data, because the elephant in the room is the legacy integration tests that we're trying to migrate here which honestly constitute the vast majority of our coverage.

@webknjaz
Copy link
Member

@AlanCoding ah, I see. Yes, that would require running that code under coverage as well. I'll think about this a bit more.

P.S. I've filed the issue on the dead code that you asked for earlier: #15803. Not pasting the files list, but I've added links to where to find said files.

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.

3 participants