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

Compare files outputted by the application record wise with some gold standard file. #4

Closed
16 tasks
rrahn opened this issue Mar 26, 2020 · 6 comments · Fixed by seqan/app-template#16
Closed
16 tasks
Assignees
Labels
ready to tackle This story was discussed and can be immidietly tackled
Milestone

Comments

@rrahn
Copy link
Contributor

rrahn commented Mar 26, 2020

Description

It should be possible to directly compare files within the application test infrastructure. The files should be compared record wise and output the record number for which the test failed for easier debugging. In addtion, it should be supported to provide a binary comparator such that it is possible to customise the record comparison.
Also it would be important to have an option that allows to check if one file is the permutation of another.

Acceptance criteria

  • test runs through without an error if the files are identical
  • if two files differ the test fails and outputs the record position for which the test fail
  • Passing a binary comparator allows to customise the comparison of records
  • gracefully fail with an exception if files could not be open.
  • files of different length fail the test with a diagnostic message that the lengths are different

Tasks

  • Implement a test macro EXPECT_FILES_EQ(file1, file2) that compares the files record wise
  • Add a second macro function EXPECT_FILES_CMP_EQ(file1, file2, binary_comparator) that compares two files record wise and uses the binary comparator to check for equality.
  • Make seqan3/test/include/seqan3/test available as include header in app tests

Definition of Done

  • Implementation and design approved
  • Unit tests pass
  • Test coverage = 100%
  • Microbenchmarks added and/or affected microbenchmarks < 5% performance drop
  • API documentation added
  • Tutorial/teaching material added
  • Test suite compiles in less than 30 seconds (on travis)
  • Changelog entry added
@rrahn rrahn added the needs refinement A story that was not discussed and/or estimated by the team yet but is planned for upcoming sprints. label Mar 26, 2020
@rrahn
Copy link
Contributor Author

rrahn commented Mar 27, 2020

What is the best design for this macro? Should it be a macro etc.?

@joergi-w
Copy link
Member

joergi-w commented Apr 6, 2020

What is the best design for this macro? Should it be a macro etc.?

We need to add information about the file type in order to choose among sequence_file_in or alignment_file_in or structure_file_in etc. I suggest to define functions in the cli test class that look like the macros, e.g. void EXPECT_SEQUENCE_FILES_EQ(std::filename::path file1, std::filename::path file2). The optional comparator could be a selection of seqan3::fields or a function on two records. It must be compatible with the record type. And there are more open questions to be discussed in a meeting, e.g. how does the algorithm know whether to read dna5, rna5, or amino acid?

@rrahn
Copy link
Contributor Author

rrahn commented Apr 6, 2020

I think you should already pass the open files. You might want to apply some filter on the comparison. For example only look at a specific fields. This can't be done with the macros that you propose or get's complicated real quick. What if you have a file that is not one of our formats? So basically we can use a range comparison for files as well. I think @marehr wanted to work on this. Let us discuss this in a meeting.

@rrahn rrahn added this to the Sprint 2 milestone Apr 13, 2020
@joergi-w joergi-w removed their assignment Apr 14, 2020
@joergi-w
Copy link
Member

What's the status here? In the last meeting we discussed that @marehr works on this, so I unassigned myself...

@smehringer
Copy link
Member

@marehr will implement a EXPECT_RANGE_EQ macro. Since our files behave like ranges, we hope that this issue is automatically solved by this macro.

@marehr I assigned this issue to you, s.t. none else starts to work on this. After finishing the macro, maybe you can check if it works for files and reassign the issue if more needs to be done.

Just a thought dump (although you probably have that in mind already): I know you want to make the macro to prettily print the differences, which may require the range to be a forward range. Files are input ranges though.

@rrahn rrahn added ready to tackle This story was discussed and can be immidietly tackled and removed needs refinement A story that was not discussed and/or estimated by the team yet but is planned for upcoming sprints. labels Apr 27, 2020
@rrahn rrahn modified the milestones: Sprint 2, Sprint 3 Apr 27, 2020
@marehr
Copy link
Member

marehr commented May 12, 2020

We had today a Sprint-Planning meeting and we concluded to use EXPECT_RANGE_EQ in the application template tests.

To do this, we need to make the include path seqan3/test/include/seqan3/test available in the app tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to tackle This story was discussed and can be immidietly tackled
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants