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] Adding cli tests for detect_breakends. #19

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

simonsasse
Copy link
Member

I am not sure about the TEST_F(cli_test, with_arguments).
It failed when I had just the result.out and then I added the result.err
I don't know why this is necessary. I think it should not produce any errors here right ?

Comment on lines +42 to +50
std::string expected_err
{
"DEL: Reference\tchr9\t70103073\tForward\tReference\tchr9\t70103147\tForward\tm13802/6999/CCS\nDone. Found 1 junctions.\n"
};
EXPECT_EQ(result.exit_code, 0);
EXPECT_EQ(result.out, expected);
EXPECT_EQ(result.err, expected_err);
Copy link
Member Author

Choose a reason for hiding this comment

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

without these lined the cli tests won't work.

Copy link
Collaborator

@Irallia Irallia 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!
Thanks a lot!
Once we have solved the problem with Travis, we can turn the draft into a real PR.

// EXPECT_EQ(result.err, "Conversion was a success. Congrats!\n");
// }
//
// there is no file output option for "detect_breakends"
Copy link
Collaborator

Choose a reason for hiding this comment

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

An output option would be nice, I will add an issue for that.

EXPECT_EQ(result.err, expected_err);
}

// There is no verbose mode for "detect_breakends".
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want a verbose option, needs a discussion. I will add an issue.

@marehr
Copy link
Member

marehr commented Oct 6, 2020

The problem why this doesn't work is, because you didn't say that the test for the breakends binary needs to be build.

--------------------------- test/cli/CMakeLists.txt ---------------------------
index 9665c48..61e3237 100644
@@ -2,8 +2,9 @@ cmake_minimum_required (VERSION 3.8)
 
 add_cli_test (fastq_to_fasta_options_test.cpp)
 target_use_datasources (fastq_to_fasta_options_test FILES in.fastq)
 
 add_cli_test(detect_breakends_cli_test.cpp)
+add_dependencies (detect_breakends_cli_test "detect_breakends")
 target_use_datasources (detect_breakends_cli_test FILES converted_bam_shorted.sam)
+
 # add_cli_test (iGenVar_options_test.cpp)
 # target_use_datasources (iGenVar_options_test FILES in.fastq)

fixes this...

test/cli/CMakeLists.txt Outdated Show resolved Hide resolved
@simonsasse
Copy link
Member Author

simonsasse commented Oct 12, 2020

If this change fixes the build problems, I'll apply it toe the other PR: #22

@Irallia Irallia marked this pull request as ready for review October 12, 2020 11:30
Copy link
Member

@joergi-w joergi-w 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, just one remark:

{
cli_test_result result = execute_app("detect_breakends",
data("converted_bam_shorted.sam"),
"detect_breakends_out_short_10.fasta");
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 an output file? If yes then its content should also be checked.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yes you're right.
I just see that there is an output in the std::out and one in a .fasta file.
Could you rename it into:

Suggested change
"detect_breakends_out_short_10.fasta");
"detect_breakends_insertion_file_out.fasta");

But I think with our small example the file is empty. I will try to figure out a better example, that we can check the content aswell.
Perhaps you are now testing whether the file is empty so that the content check is already there and we just have to adjust it later.

Copy link
Collaborator

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

Sorry for that. @joergi-w was right. I overlooked the fact that we are writing an output file. I'm still trying to understand the entire code. 😅

{
cli_test_result result = execute_app("detect_breakends",
data("converted_bam_shorted.sam"),
"detect_breakends_out_short_10.fasta");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yes you're right.
I just see that there is an output in the std::out and one in a .fasta file.
Could you rename it into:

Suggested change
"detect_breakends_out_short_10.fasta");
"detect_breakends_insertion_file_out.fasta");

But I think with our small example the file is empty. I will try to figure out a better example, that we can check the content aswell.
Perhaps you are now testing whether the file is empty so that the content check is already there and we just have to adjust it later.

Copy link
Collaborator

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

Perfect, thank you.
I created a new Issue for the output file problem: #33 .

@Irallia Irallia merged commit aa6a5ae into seqan:master Oct 19, 2020
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.

None yet

4 participants