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

Add tests for the examples #61

Merged
merged 7 commits into from Dec 30, 2017
Merged

Add tests for the examples #61

merged 7 commits into from Dec 30, 2017

Conversation

tom-tan
Copy link
Member

@tom-tan tom-tan commented Dec 20, 2017

This request partially solve #37 by adding tests for the examples.

I do not add tests for the following sections:

  • Section 2, 3, 7, 9, 11 (2nd and 3rd examples), 12, 13 and 14
    • They depend on side-effects. It should be fixed to use outputs instead of side-effects.
    • I added test cases for Section 2 and 3 to check the examples in these sections runnable.
  • Section 11 (1st example)
    • We expect it should fails but cwltest cannot test such cases. We should fix cwltest for known failure testing.
    • I added a test case for Section 11 (1st example) because the blocker is solved by cwltest#38.
  • Section 19 and 20

Here are some notes for tests.

  • I added several files to make the example in section 17 runnable.
  • I skipped checking the checksum and size of the resulted files for section 8, 15, 21 and 22 because they depends on the external compiler and it makes the tests fragile if we check the checksum and size.
    • Now we checks checksums and sizes for these sections.
  • (edited) I skipped checking the checksum and size for the examples for section 16 and 17 because the outputs contain randomly generated paths and resulted checksums and sizes can be varied.
  • In this request, the test for section 10 expects that the glob in outputs returns files in the deterministic order. I am not sure it is a defined behavior or a system-dependent behavior.
    • Depending on the order of globing is harmless because it is deterministic (See cwltool#587).

@tom-tan tom-tan mentioned this pull request Dec 20, 2017
21 tasks
@tom-tan
Copy link
Member Author

tom-tan commented Dec 20, 2017

I also enabled docker for Travis CI because some examples depend on it.

@mr-c
Copy link
Member

mr-c commented Dec 20, 2017

Section 2, 3, 7, 9, 11 (2nd and 3rd examples), 12, 13 and 14
Yes, but we don't introduce outputs until Section 4, so the first two should stay as they are

I agree that Sections 7 onward should use outputs appropriately

I skipped checking the checksum and size of the resulted files for section 8, 15, 21 and 22 because they depends on the external compiler and it makes the tests fragile if we check the checksum and size.

Since these are all after Episode 7 "Running Tools Inside Docker" then we can add a DockerRequirement and use that to test for a specific checksum and size

@mr-c
Copy link
Member

mr-c commented Dec 20, 2017

(edited) I skipped checking the checksum and size for the examples for section 16 and 17 because the outputs contain randomly generated paths and resulted checksums and sizes can be varied.

Again, we can add a DockerRequirement to the hints for these, or pass --default-container debian:8 only during testing

@tom-tan
Copy link
Member Author

tom-tan commented Dec 21, 2017

Yes, but we don't introduce outputs until Section 4, so the first two should stay as they are

I see. I will add tests for section 2 and 3 that just check that the execution will succeed.

Since these are all after Episode 7 "Running Tools Inside Docker" then we can add a DockerRequirement and use that to test for a specific checksum and size

That will be nice. Once these tests use docker containers, we will add more precise checking for checksum and size.

By the way, is it reasonable to use java:7-jdk container? It is officially deprecated (description for library/java).

Again, we can add a DockerRequirement to the hints for these, or pass --default-container debian:8 only during testing

I prefer to use hints because it makes easier for readers and also makes a test in Travis CI consistent with the reader's environment.

I will update the PR to reflect your comment.

@mr-c
Copy link
Member

mr-c commented Dec 22, 2017

By the way, is it reasonable to use java:7-jdk container? It is officially deprecated (description for library/java).

Seems that we should switch to openjdk:7-jdk in all the common-workflow-language repos that have references to java:7-jdk (or similar).

I prefer to use hints because it makes easier for readers and also makes a test in Travis CI consistent with the reader's environment.

Yes, they can go in the hints section. We should have named DockerRequirement just Docker or Container: currently in CWL v1.x.x it is a bit confusing.

@tom-tan
Copy link
Member Author

tom-tan commented Dec 25, 2017

Done!

  • I added tests for Section 2 and 3 to check they are successfully runnable.
  • I added a test for the first example in Section 11 (Related: cwltest#38).
  • I removed the comment for Section 10 because now the order of globbing is deterministic (cwltool#587).

Other issues such as side effects and docker images will be solved by other requests.

@mr-c
Copy link
Member

mr-c commented Dec 26, 2017

:-) Let me know when you want a final review and merge.

@tom-tan
Copy link
Member Author

tom-tan commented Dec 30, 2017

I added checking checksums and sizes for Section 8, 15, 21 and 22 because now they uses docker images with precise version tag.

This request passes CI and now it is ready to merge!

@mr-c mr-c merged commit b716213 into common-workflow-language:gh-pages Dec 30, 2017
@mr-c
Copy link
Member

mr-c commented Dec 30, 2017

Wonderful, thanks!

@tom-tan tom-tan deleted the add-tests branch January 4, 2018 23:19
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

2 participants