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

Remove dss-prod from testing #522

Merged
merged 2 commits into from
May 11, 2020
Merged

Remove dss-prod from testing #522

merged 2 commits into from
May 11, 2020

Conversation

amarjandu
Copy link
Contributor

@amarjandu amarjandu commented May 7, 2020

Removes the potential for dss-tests to be run against dss-prod when on an operators machine.

See: DataBiosphere/azul#1768

@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #522 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #522   +/-   ##
=======================================
  Coverage   85.63%   85.63%           
=======================================
  Files          40       40           
  Lines        1893     1893           
=======================================
  Hits         1621     1621           
  Misses        272      272           

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 82a8cec...1ab2575. Read the comment docs.

@melainalegaspi melainalegaspi added the orange Done by the Azul, Data Browser and Portal team label May 8, 2020
Comment on lines 33 to 43
args = ["dss", "get-bundles-all", "--replica", "aws",
"--per-page", "10", "--no-paginate"]
with CapturingIO('stdout') as stdout:
hca.cli.main(args)
result = json.loads(stdout.captured())
try:
assert("https://dss.data.humancellatlas.org" not in result["dss_api"])
except AssertionError:
print('\nPlease change the DSS swagger endpoint in the HCA config file away from `dss-prod`\n'
'For more information see: `https://github.com/HumanCellAtlas/dcp-cli#development`')
exit(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you also do this by inspecting / importing the DSS config? Then you could check the stage without needing to make an API call.

Also I think this is a round-about use of assert. A conditional would be more concise and clear.

Calling exit here seems like overkill. If only these tests use the outside resource, then there's no need to quit the whole process; other tests should be allowed to run. If other tests (not in TestDssCLI) are counting on this fail mechanism then I'd say that's a problem. Unittest doesn't guarantee the order or test evaluation.

Since you don't need to call exit(), then I think it's sufficient just to raise ValueError() here. This would allow other tests (if they exist) to still run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair points, I've incorporated them all. :)

@jessebrennan jessebrennan removed their assignment May 9, 2020
Copy link
Collaborator

@jessebrennan jessebrennan left a comment

Choose a reason for hiding this comment

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

If you decide to squash these, check out git rebase -i --autosuash.

@jessebrennan jessebrennan removed their assignment May 11, 2020
@amarjandu amarjandu merged commit 330caa6 into master May 11, 2020
@hannes-ucsc
Copy link
Contributor

If the association to the connected issue isn't clear: this PR prevents the creation of more bad bundles by DSS tests. And it's the goal of #1768 to remove those bundles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
orange Done by the Azul, Data Browser and Portal team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants