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

Move EUnit to CT #115

Merged
merged 2 commits into from
Jun 30, 2024
Merged

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Jul 27, 2023

Closes #101.

Depends on the CI file updates from #114, so shall be rebased and updated (if required) after that one.

In CT (this pull request):

  |----------------------------------|------------|
  |                          module  |  coverage  |
  |----------------------------------|------------|
  |                            elli  |       77%  |
  |           elli_example_callback  |       90%  |
  |  elli_example_callback_handover  |      100%  |
  |         elli_example_middleware  |      100%  |
  |                       elli_http  |       67%  |
  |                 elli_middleware  |      100%  |
  |        elli_middleware_compress  |      100%  |
  |                    elli_request  |       78%  |
  |                   elli_sendfile  |       62%  |
  |                        elli_tcp  |       86%  |
  |                       elli_test  |      100%  |
  |                       elli_util  |      100%  |
  |----------------------------------|------------|
  |                           total  |       76%  |
  |----------------------------------|------------|

In EUnit (main)

  |----------------------------------|------------|
  |                          module  |  coverage  |
  |----------------------------------|------------|
  |                            elli  |       77%  |
  |           elli_example_callback  |       90%  |
  |  elli_example_callback_handover  |      100%  |
  |         elli_example_middleware  |      100%  |
  |                       elli_http  |       68%  |
  |                 elli_middleware  |      100%  |
  |        elli_middleware_compress  |      100%  |
  |                    elli_request  |       78%  |
  |                   elli_sendfile  |       62%  |
  |                        elli_tcp  |       86%  |
  |                       elli_test  |      100%  |
  |                       elli_util  |      100%  |
  |----------------------------------|------------|
  |                           total  |       76%  |
  |----------------------------------|------------|

I tracked the differences for the elli_http module and they're only related to the missing test code (7 lines in the previous code). In elli_test this is not noticeable because we covered 100% and now cover 100% still...

From the commit notes:

Move tests from EUnit to Common Test
Notice the use of README.md from the linked `priv` folder

Files were mostly renamed and then:
- added init/end_per_suite (from setup/teardown)
- added init/end_per_testcase (from elli_test_)
- removed _test suffix from exported function names
- moved rebar.config test options to test profile
Add a shareable (example + tests) symbolic link
Used as filename:join(code:priv_dir(elli), "README.md") for
single reference...
Prepare for CT
- deps and ebin are rebar 2
- ignore *.beam is not required, since _build
- .eunit is legacy
- pre-hook lint (for eunit) is removed and made explicit in the CI file
- main.yml is tweaked to run static analysis first and tests later

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 27, 2023

It's possible the tests fail for this one, and it also doesn't move up the supported OTP version. I'll copy that over from #112 and check what needs to be adapted.

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.08%. Comparing base (05e24b1) to head (9fe89e7).
Report is 31 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
- Coverage   76.30%   76.08%   -0.23%     
==========================================
  Files          12       12              
  Lines         764      740      -24     
==========================================
- Hits          583      563      -20     
+ Misses        181      177       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 27, 2023

CI failing for OTP 26. I'll try to replicate locally.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jul 28, 2023

Removed OTP 26 from scope, since there're SSL -related issues to solve. Should be moved to a new PR, I guess...

@paulo-ferraz-oliveira
Copy link
Contributor Author

Is anything lacking for this one to get merged? I can then rebase and update the other ones.

@tsloughter
Copy link
Member

Only issue with these from my perspective has been their size (even if it looks like many of the commits are the same across the PRs, its still that getting one merged means being confident with changes to a lot of files) and I just haven't had the time to look them over close enough to feel comfortable.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Aug 2, 2023

I understand. I'll try to rebase/squash, etc. to ease review and maybe that can remove some commits that moved back and forth... @tsloughter, I think I did what I wanted already, in all the pull requests I have opened. The first one needing merge will be #114, so the other ones can "inherit" the new maintenance range and updates to CI files.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Moved to draft. These don't need to be reviewed before #114, after which I'll rebase.

- deps and ebin are rebar 2
- ignore *.beam is not required, since _build
- .eunit is legacy
- pre-hook lint (for eunit) is removed and made explicit in the CI file
- main.yml is tweaked to run static analysis first and tests later
Notice the use of README.md from the linked `priv` folder

Files were mostly renamed and then:
- added init/end_per_suite (from setup/teardown)
- added init/end_per_testcase (from elli_test_)
- removed _test suffix from exported function names
- moved rebar.config test options to test profile
- added a shareable (example + tests) symbolic link, used as
  filename:join(code:priv_dir(elli), "README.md") for
  single reference...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We go from eunit+dialyzer to xref+dialyzer+lint+ct+cover

Note: eunit was doing lint, as per rebar.config, but it's probably better to have it explicitly as a CI step.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@tsloughter

@tsloughter tsloughter merged commit 1512481 into elli-lib:main Jun 30, 2024
2 checks passed
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/ct branch June 30, 2024 11:51
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.

Move tests to a CT suite
4 participants