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

[INFRA] Merge App-Template and add codecoverage #118

Merged
merged 24 commits into from
May 12, 2021

Conversation

Irallia
Copy link
Collaborator

@Irallia Irallia commented May 10, 2021

Resolves #116 (add codecoverage)
Resolves #112 (write more tests, until codecove is higher than 85%)
And with this it resolves #30 (EPIC: Write API & CLI tests for all functions)

This PR merges the App-Template into iGenVar again. Thus we get the latest updates of the app-template including codecoverage.

rrahn and others added 22 commits December 4, 2020 23:34
…flags.

Changes:
* Link against google test targets with "d" suffix in the target name when building in debug mode.
* Use the same cmake flags to configure googletest as used for the project

These changes make sure that the google test target is configured and
built with the same flags as given to the top level cmake file.
This avoids some incompatibilities, e.g. that the
debug targets are not found by the test binaries,
when build in debug mode.
[INFRA] Allows to configure googletest with the same top-level cmake flags.
[MISC] Make SeqAn3 test includes available in api test
* [INFRA] cmake: use seqan3 googletest

A better attempt at fixing seqan/app-template#37

* test

* test2

* like this?

Co-authored-by: Enrico Seiler <enrico.seiler@hotmail.de>
[FIX] using old homebrew version
Co-authored-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Co-authored-by: David Heller <heller_d@molgen.mpg.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia changed the title Infra/merge apptemplate [INFRA] Merge App-Template May 10, 2021
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia force-pushed the INFRA/merge_apptemplate branch 2 times, most recently from 9c17d1a to 5ac6403 Compare May 10, 2021 19:47
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@736d46c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #118   +/-   ##
=========================================
  Coverage          ?   85.45%           
=========================================
  Files             ?        4           
  Lines             ?      110           
  Branches          ?        0           
=========================================
  Hits              ?       94           
  Misses            ?       16           
  Partials          ?        0           

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 736d46c...3761e1a. Read the comment docs.

@Irallia Irallia changed the title [INFRA] Merge App-Template [INFRA] Merge App-Template and add codecoverage May 10, 2021
Comment on lines +34 to +37
# - name: "gcc11"
# cxx: "g++-11"
# cc: "gcc-11"
# build_type: Release
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for gcc11 on linux the tests didn't run through...

[ RUN      ] iGenVar_cli_test.test_unknown_argument
../../../src/test/cli/iGenVar_cli_test.cpp:308: Failure
Expected equality of these values:
  result.err
Error: ch is: "[Error] You have chosen an invalid input value: 9. Please use one of: [read_depth,read_pairs,split_read,3,1,2,cigar_string,0]\n"
  expected_err
Error: ch is: "[Error] You have chosen an invalid input value: 9. Please use one of: [read_depth,3,read_pairs,2,split_read,1,cigar_string,0]\n"
[  FAILED  ] iGenVar_cli_test.test_unknown_argument (4 ms)

Copy link
Member

@eseiler eseiler May 10, 2021

Choose a reason for hiding this comment

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

The enumeration_names is a map, so the order of keys is not fixed and may differ for each compiler/os/etc.

You could, in a follow up PR, for example change the test to tokenise (split) the [read_depth,...,...,..,] part and check if everything is there. Something like splitting at ,, putting the strings into a set and then comparing two sets would work.

Or just have a fixed order in the help/error message, which would require changes in seqan3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-> #119

@Irallia Irallia requested a review from eseiler May 10, 2021 20:03
@@ -8,16 +8,16 @@ if (${DOXYGEN_FOUND})

# Configure doxygen options.
set (APP_TEMPLATE_DOXYFILE_IN ${CMAKE_SOURCE_DIR}/doc/doxygen_cfg)
set (APP_TEMPLATE_DOXYGEN_OUTPUT_DIR "${PROJECT_BINARY_DIR}/doc")
set (IGENVAR_DOXYGEN_OUTPUT_DIR "${PROJECT_BINARY_DIR}/doc")
Copy link
Member

Choose a reason for hiding this comment

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

still APP_TEMPLATE_DOXYGEN_OUTPUT_DIR in the doxyfile.

Copy link
Collaborator

@joshuak94 joshuak94 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia merged commit db105d3 into seqan:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants