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

Test Coverage Reporting #14343

Merged
merged 1 commit into from
Apr 7, 2025
Merged

Conversation

maennchen
Copy link
Member

@maennchen maennchen commented Mar 18, 2025

Changes

Implements test coverage reporting.

Requirements

  • Can show coverage summary in CI
  • Offers details for download (HTML?)
  • Can show coverage summary in CLI
  • Follows same rules as mix test.coverage for reporting (generated lines etc.)

TODOs / Issues

@maennchen maennchen force-pushed the jm/test_coverage branch 2 times, most recently from d6198ed to 2149314 Compare March 18, 2025 17:44
@maennchen
Copy link
Member Author

@josevalim Do you have any preference on how to deal with the FunctionClauseError issues?

(See https://github.com/elixir-lang/elixir/actions/runs/13929976188/job/38984368756?pr=14343)

@josevalim
Copy link
Member

@maennchen probably the best option is to check if coverage is enabled and perform only a partial assertion (i.e. break this assertion in two), the second one wrapped by a conditional.

@maennchen
Copy link
Member Author

@josevalim Ok, I will do that. Do you btw. know the reason why this doesn't work while cover compiled?

@josevalim
Copy link
Member

@josevalim Ok, I will do that. Do you btw. know the reason why this doesn't work while cover compiled?

We traverse the stored ast when analyzing the error message but when cover compiling the information is lost.

@maennchen
Copy link
Member Author

@josevalim I got it down to one test failure:

  1) test structs in lists (ExUnit.DiffTest)
     test/ex_unit/diff_test.exs:943
     Assertion with =~ failed
     code:  assert diff_left =~ expected_left
     left:  "[\n  -%ExUnit.DiffTest.Customer{\n    address: %{\"street\" => \"123 Main St\", \"zip\" => \"62701\"},\n    language: \"en-US\",\n    age: 30,\n    first_name: \"John\",\n    last_name: \"Doe\",\n    notifications: true\n  }-\n]"
     right: "[\n  -%ExUnit.DiffTest.Customer{\n    address: %{\"street\" => \"123 Main St\", \"zip\" => \"62701\"},\n    age: 30,\n    first_name: \"John\",\n    language: \"en-US\",\n    last_name: \"Doe\",\n    notifications: true\n  }-\n]"
     stacktrace:
       test/ex_unit/diff_test.exs:123: ExUnit.DiffTest.refute_diff/5
       test/ex_unit/diff_test.exs:956: (test)

The ExUnit.DiffTest.Customer module is defined inside the test and therefore should also not be cover compiled. Do you have a clue why the order is different?

@josevalim
Copy link
Member

@maennchen i would have to investigate locally which order is being printed. It seems it was supposed to be alphabetical.

@maennchen maennchen force-pushed the jm/test_coverage branch 5 times, most recently from e4420f0 to 64f4c8d Compare March 24, 2025 15:33
@maennchen maennchen force-pushed the jm/test_coverage branch 2 times, most recently from 8d07aca to 66ee072 Compare March 31, 2025 12:04
@maennchen maennchen marked this pull request as ready for review March 31, 2025 12:04
@maennchen
Copy link
Member Author

Splitting PR into multiple PRs. Marking as draft until done.

@maennchen maennchen force-pushed the jm/test_coverage branch 2 times, most recently from 4ce9a07 to 0901ddd Compare April 7, 2025 14:12
@maennchen maennchen marked this pull request as ready for review April 7, 2025 14:12
@maennchen maennchen marked this pull request as draft April 7, 2025 15:59
@maennchen maennchen force-pushed the jm/test_coverage branch 2 times, most recently from 642dfde to b325ae0 Compare April 7, 2025 17:16
@maennchen maennchen marked this pull request as ready for review April 7, 2025 17:27
cover/ex_unit_elixir.coverdata:
$(Q) COVER="1" $(MAKE) test_stdlib
Copy link
Member

Choose a reason for hiding this comment

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

Is this a left-over?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is intentionally as a dependency of the cover target.

The idea is that you can call make cover and it will run all the tests with coverage. (I tried to make it simple for non-regular contributors as well to get the coverage report.)

@maennchen maennchen force-pushed the jm/test_coverage branch 2 times, most recently from 17cee14 to 582ffec Compare April 7, 2025 18:04
@josevalim
Copy link
Member

Thank you, codewise, this looks good.

We just need to improve the quality of the report a bit. For this PR, I think the only thing we need to do is to exclude deprecated modules from the report. Is this possible? In theory we should be able to filter by this:

match? {:docs_v1, _, _, _, _, %{deprecated: _}}, Code.fetch_docs(mod)

But I don't know if the best moment is when running tests or when assembling coverage. I would also skip the Kernel module, because most of it is inlined by the compiler anyway.

The other tests we should improve the coverage through separate pull requests.

What are your thoughts?

@maennchen
Copy link
Member Author

On Deprecation: I'm trying it out. I would directly exclude them from cover compiling. At that stage I should still have access to the unmodified AST including docs.

On the Kernel: Looking through the HTML, I think it still valuable to include it in the reporting. Stuff like this is helpful when writing tests and the coverage is at 80% which isn't that low.:

image

However with Kernel.SpecialForms, we can make an exception. I don't think that module is called anywhere.

I would however prefer to draw the line after the deprecation exclusion for this specific PR. The rest I would do in separate PRs as you propose.

@maennchen
Copy link
Member Author

PR is updated with exclusion of deprecated modules.

@josevalim josevalim merged commit f3e8ceb into elixir-lang:main Apr 7, 2025
11 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim
Copy link
Member

You are right. I did a mistake between Kernel.CLI and Kernel. Kernel.CLI is tested... but it uses separate OS processes, so it does not get measured. :)

@maennchen maennchen deleted the jm/test_coverage branch April 8, 2025 04:38
@maennchen
Copy link
Member Author

FYI on the OpenSSF Best Practices Badge:

There's some requirements in the higher levels (Silver / Gold). While I don't suggest to try to fulfill them at this time, this may be relevant to prepare for it a bit longer term.

Silver:

  • The project MUST have FLOSS automated test suite(s) that provide at least 80% statement coverage if there is at least one FLOSS tool that can measure this criterion in the selected language. [test_statement_coverage80]

Gold:

  • The project MUST have FLOSS automated test suite(s) that provide at least 90% statement coverage if there is at least one FLOSS tool that can measure this criterion in the selected language. [test_statement_coverage90]
  • The project MUST have FLOSS automated test suite(s) that provide at least 80% branch coverage if there is at least one FLOSS tool that can measure this criterion in the selected language. [test_branch_coverage80]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants