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

Fix tests for upgrade_document #85

Merged
merged 3 commits into from
Nov 16, 2021
Merged

Fix tests for upgrade_document #85

merged 3 commits into from
Nov 16, 2021

Conversation

tom-tan
Copy link
Member

@tom-tan tom-tan commented Nov 15, 2021

This request fixes tests that were added by #79 (introduced by me :-( ).
They should check the upgraded document and the expected document but they do not in the current implementation.

I also change _v1_0_to_v1_1 to add LoadListingRequirement and NetworkAccess to requirements rather than hints not to break the behavior of given CWL documents. See listing_deep1.cwl for example.

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #85 (259a622) into main (e2a05df) will increase coverage by 3.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
+ Coverage   72.27%   75.51%   +3.23%     
==========================================
  Files           4        4              
  Lines         523      535      +12     
==========================================
+ Hits          378      404      +26     
+ Misses        145      131      -14     
Impacted Files Coverage Δ
cwlupgrader/main.py 72.42% <100.00%> (+2.94%) ⬆️
tests/test_complete.py 100.00% <100.00%> (ø)

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 e2a05df...259a622. Read the comment docs.

@mr-c
Copy link
Member

mr-c commented Nov 15, 2021

This request fixes tests that were added by #79 (introduced by me :-( ). They should check the upgraded document and the expected document but they do not in the current implementation.

👍👍

I also change _v1_0_to_v1_1 to add LoadListingRequirement and NetworkAccess to requirements rather than hints not to break the behavior of given CWL documents. See listing_deep1.cwl for example.

Interesting. This does not match the behavior of the reference runner. https://github.com/common-workflow-language/cwltool/blob/352daf6d53f12df4cd960c0343ef659924aa9712/cwltool/update.py#L127

@tetron Do you remember why we put these as hints instead of requirements?

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

A couple lines without test coverage.

cwlupgrader/main.py Show resolved Hide resolved
cwlupgrader/main.py Show resolved Hide resolved
cwlupgrader/main.py Show resolved Hide resolved
@tom-tan
Copy link
Member Author

tom-tan commented Nov 16, 2021

ping!

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

LGTM, but @tetron needs to share his opinion about adding LoadListingRequirement and NetworkAccess to reqs instead of hints when upgrading v1.0 → v1.1

@mr-c mr-c merged commit df84c39 into main Nov 16, 2021
@mr-c mr-c deleted the fix-test branch November 16, 2021 16:02
@mr-c
Copy link
Member

mr-c commented Nov 16, 2021

@tom-tan , Peter says "sure" :-)

https://matrix.to/#/!RQMxrGNGkeDmWHOaEs:gitter.im/$2uVLM10OOgig25VMO8ZY2kAEQaiI05nYKW1ijblsC2Y

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.

2 participants