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] Refactor api tests #169

Merged
merged 4 commits into from
Oct 19, 2021
Merged

Conversation

Irallia
Copy link
Collaborator

@Irallia Irallia commented Oct 4, 2021

Some refactoring for #168 .

@Irallia Irallia self-assigned this Oct 4, 2021
test/api/api_test.hpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #169 (9a7e65d) into master (d14e155) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #169   +/-   ##
=======================================
  Coverage   96.48%   96.48%           
=======================================
  Files          19       19           
  Lines         797      797           
=======================================
  Hits          769      769           
  Misses         28       28           
Impacted Files Coverage Δ
src/variant_detection/variant_detection.cpp 93.97% <ø> (ø)

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 d14e155...9a7e65d. Read the comment docs.

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia requested review from a team and SGSSGene and removed request for a team October 8, 2021 09:31
Copy link

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

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

There might be subtle problem with gVerbose = true. By default it is initialized to false. Depending on the unit test order, it might be set to true by a previous unittest.
So it is necessary to reset it to false.
I think this could be a great use case of the use of a scope guard (Idea taken from here https://stackoverflow.com/questions/28729545/abusing-c11-unique-ptr-to-execute-code-upon-leaving-the-scope):
By implementing a scope guard function:

template<class F> auto scope_guard(F&& f) {
    return std::unique_ptr<void, std::decay<F>::type>{(void*)1, std::forward<F>(f)};
}

You can now protect your code by writting:

TEST(...) {
    auto verboseGuard = scope_guard([]{gVerbose = false; });
    gVerbose = true;
    ....
}

Or you define even a help construct for that:

template<class F> auto scope_guard(F&& f) {
    return std::unique_ptr<void, std::decay<F>::type>{(void*)1, std::forward<F>(f)};
}
auto guard_verbose (bool verbose) {
    auto lastState = gVerbose;
    gVerbose = verbose;
    return scope_guard([=]{gVerbose = lastState; });
}

This would allow you to write:

TEST(...) {
    auto verboseGuard = verbose_guard(true); // will reset back to the original state, after leaving this scope
    ....
}

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Copy link

@SGSSGene SGSSGene 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 :-)

@Irallia Irallia requested a review from joergi-w October 8, 2021 11:20
@Irallia Irallia marked this pull request as ready for review October 8, 2021 11:20
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
// Uses a unique_ptr to implement a scope_guard for guarding the gVerbose state
auto verbose_guard(bool verbose)
{
// Lambda is being executed when the scope of the unique_ptr is being left
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Lambda is being executed when the scope of the unique_ptr is being left
// Lambda is executed when the scope of the unique_ptr ends

}

// This is a helper function for debugging, it prints junctions for comparing
void print_compare_junction_vectors(std::vector<Junction> const & junctions_expected_res,
Copy link
Member

Choose a reason for hiding this comment

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

#include <vector> is missing

@@ -426,178 +446,182 @@ TEST(junction_detection, analyze_sa_tag)
1000, // default partition_max_distance
0.5}; // default hierarchical_clustering_cutoff

std::vector<Junction> junctions_res{};
{ // Example 1
seqan3::debug_stream << "----------------------------------First Example:----------------------------------\n";
Copy link
Member

Choose a reason for hiding this comment

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

This is printed on each test execution – is this intended (even if print_compare_junction_vectors is just a comment)? The same below for the second example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, because it splits the different examples. I think a few prints give a little more clarity, as there are other printouts while executing the single tests.

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia merged commit e2af736 into seqan:master Oct 19, 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
Development

Successfully merging this pull request may close these issues.

3 participants