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 many issues with examples #669

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Fix many issues with examples #669

merged 1 commit into from
Jan 24, 2025

Conversation

jdidion
Copy link
Collaborator

@jdidion jdidion commented Jun 26, 2024

Closes #653.
Closes #654.
Closes #661.
Closes #662.
Closes #663.
Closes #664.
Closes #665.
Closes #666.
Closes #667.
Closes #668.

Checklist

  • Pull request details were added to CHANGELOG.md

@jdidion jdidion changed the title Fix may issues with tests Fix may issues with examples Jun 26, 2024
@stxue1 stxue1 mentioned this pull request Jul 3, 2024
2 tasks
@jdidion
Copy link
Collaborator Author

jdidion commented Jul 24, 2024

@stxue1 @adamnovak would also appreciate your reviews

Copy link
Collaborator

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

This looks like everything should work.

SPEC.md Show resolved Hide resolved
SPEC.md Show resolved Hide resolved
SPEC.md Outdated Show resolved Hide resolved
SPEC.md Show resolved Hide resolved
SPEC.md Outdated Show resolved Hide resolved
SPEC.md Show resolved Hide resolved
SPEC.md Outdated Show resolved Hide resolved
@jdidion jdidion changed the title Fix may issues with examples Fix many issues with examples Jul 26, 2024
SPEC.md Show resolved Hide resolved
@stxue1
Copy link
Collaborator

stxue1 commented Sep 30, 2024

The unit test test_hints wc command is malformed:

wdl/SPEC.md

Line 4584 in 20ff54d

wc -l ~{foo}

And should be:

  wc -l < ~{foo}

The expected output should change from 3 to 2 as well:

wdl/SPEC.md

Line 4619 in 20ff54d

"test_hints.num_lines": 3

{
  "test_hints.num_lines": 2
}

@stxue1
Copy link
Collaborator

stxue1 commented Sep 30, 2024

input_hint is missing an output:

wdl/SPEC.md

Lines 4703 to 4705 in 20ff54d

```json
{}
```

I'm not too certain what the expected output should be. MiniWDL currently returns

{
    "input_hint.experience": []
}

@claymcleod
Copy link
Collaborator

input_hint is missing an output:

wdl/SPEC.md

Lines 4703 to 4705 in 20ff54d

```json
{}
```

I'm not too certain what the expected output should be. MiniWDL currently returns

{
    "input_hint.experience": []
}

This has also been resolved.

Based on this, I think the PR is good to go, and I'm going to merge it. This PR has sat for a second, so I did the best I could with making sure nothing was missed, but feel free to file another issue if anything wasn't pulled in correctly.

This commit fixes multiple issues in the `v1.1.*` specification. Many
(if not all) of the tests were surfaced and reported by @stxue1. The
fixes for these tests were a combination of effort from @stxue1 and
@jdidion. @adamnovak added feedback on the PR that further refined the
tests. @claymcleod did the final review and merge of the pull request
in #669.

Closes #653.
Closes #654.
Closes #661.
Closes #662.
Closes #663.
Closes #664.
Closes #665.
Closes #666.
Closes #667.
Closes #668.

Co-authored-by: Clay McLeod <[email protected]>
Co-authored-by: John Didion <[email protected]>
Co-authored-by: Adam Novak <[email protected]>
@claymcleod claymcleod merged commit d5fb794 into wdl-1.1 Jan 24, 2025
@claymcleod claymcleod deleted the wdl-1.1.3 branch January 24, 2025 00:15
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.

4 participants