Conversation
vagkaratzas
left a comment
vagkaratzas
left a comment
There was a problem hiding this comment.
Left a comment for snapshots. Also, the new json test file is 5.4 MB...Can you get a waaaaaaaay smaller one please? Should be goot to ship otherwise
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot(process.out.gff).match("gff") }, | ||
| { assert snapshot(process.out.versions_mgnifypipelinestoolkit).match("versions") } |
There was a problem hiding this comment.
Why are you creating multiple snapshots per test file? You can put all assertions in one snapshot per test case. Same for the stub tests.
Example:
then {
assert process.success
assertAll(
{ assert snapshot(
process.out.gff,
process.out.findAll { key, val -> key.startsWith("versions")}
).match() }
)
}
This then block should be the standard.
There was a problem hiding this comment.
Yes, that was a kind of a workaround before the schemas were updated for using topics. I had fix it now
mberacochea
left a comment
mberacochea
left a comment
There was a problem hiding this comment.
Thank you @Ales-ibt. It looks good, I left some comments
modules/ebi-metagenomics/antismash/json2gff/tests/data/test_antismash.json
Outdated
Show resolved
Hide resolved
|
@vagkaratzas and @mberacochea |
mberacochea
left a comment
mberacochea
left a comment
There was a problem hiding this comment.
One thing to decide before merging... if we need/want to revert all the changes the linter did on the antiSMASH test files
| 3 # kernel parameter -d | ||
| 0.00070915 # kernel parameter -g | ||
| 1 # kernel parameter -s | ||
| 1 # kernel parameter -r | ||
| empty# kernel parameter -u | ||
| 408 # highest feature index | ||
| 946 # number of training documents | ||
| 135 # number of support vectors plus 1 |
There was a problem hiding this comment.
Prettier modified a bunch of files... loads of them. Maybe we should revert them?
There was a problem hiding this comment.
But tests are passing. I am wondering why these changes weren't made before by pre-commit?
There was a problem hiding this comment.
they seem harmless.. so let's merge
3 participants
I have created a module that we are using in multiple pipelines to transform the antismash result from json format into gff format. To put the module in the right place, I had to move the antismash module one level inside the
antismashdirectory, so you will see the renamed files in the list of files to review. Many of those files have changes due to pre-commit like space removing and so on. Please ignore them and take a look just to the files in theantismash/json2gffdirectory.Thanks for reviewing!