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

Closes #1279: Add code coverage of unit tests as MAKE target #1280

Merged
merged 2 commits into from
Sep 6, 2022
Merged

Closes #1279: Add code coverage of unit tests as MAKE target #1280

merged 2 commits into from
Sep 6, 2022

Conversation

aryoda
Copy link
Contributor

@aryoda aryoda commented Sep 2, 2022

I have implemented my DEV feature request now.

To test the new make target:

cd common
./configure
make coverage

@emtiu Could you please check and merge my PR? THX!

@aryoda aryoda changed the title Closes #1279: Add code coverage of unit tetss as MAKE target Closes #1279: Add code coverage of unit tests as MAKE target Sep 2, 2022
@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2022

Please put this PR on hold, I

  • forgot to put the common/plugins and qt/plugins folders under observation
  • forgot to extend the make clean target (remove the coverage data and report files again).

Furthermore I am wondering what coveralls.io is measuring (click on the coverage badge on the README file).
The reported value seems to high (may be caused by the unit test files which are considered too in the overall percentage which is false IMHO - the unit test coverage should be separated from the app code coverage because I only want to discover unit test fragments that never get executed due to an failure or logical coding error).

…h fixed make clean target and forgotten plugins source files)
@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2022

@emtiu OK, I have fixed the missing make clean target and included the forgotten code files from the "plugins" folders. You can merge now I everything is fine.
BTW: I have not rebased into one single "commit", is this a policy here?

@buhtz
Copy link
Member

buhtz commented Sep 2, 2022

BTW: I have not rebased into one single "commit", is this a policy here?

Where good point! 😄 There is no policy yet. Just another TODO bullet on my list.

As an exception the current PR would be fine for me.
But because of that situation usually a PR should not created against master (or main). I would say we can discuss a "policy" on the mailing list when Germar granted us access as maintainers.

Until then and as a temporary "workaround" I would suggest to create PR's only against a new branch: So aryoda:1279_make_coverage should be created against bit-team:1279_make_coverage.

@aryoda
Copy link
Contributor Author

aryoda commented Sep 2, 2022

I would suggest to create PR's only against a new branch: So aryoda:1279_make_coverage should be created against bit-team:1279_make_coverage.

Is there a reason to send pull requests against a new upstream branch (how could I create it BTW)?
IMHO this makes things more complicated until there is a good reason to do so. Just my 5 cents...

@buhtz
Copy link
Member

buhtz commented Sep 2, 2022

Is there a reason to send pull requests against a new upstream branch

Yes. But let's discuss that later on the dev-bit list. There are several options ("git workflows") to handle such cases.

(how could I create it BTW)?

Ah... I see it is not possible on GitHub or not possible because of our rights or BIT project settings. So not possible yet. Sorry my fault.

Technically it is (sometimes) possible to create a new branch when creating a PR.
image

EDIT: It seems to me that GitHub offers something like "squash merging" what would solve "the problem". But only @emtiu can tell us if the project is configured for it.

@emtiu emtiu merged commit 6d4df98 into bit-team:master Sep 6, 2022
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