catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

Fix typos #597

Closed JuliaSprenger closed 2 years ago

JuliaSprenger commented 2 years ago

Motivation

There is no dedicated test requirement document specified, so the full requirements should include the requirements for test. This PR also fixes a couple of typos on the way.

Checklist

Update

The requirement issue is already fixed in main, this PR only adds typo fixes

CodyCBakerPhD commented 2 years ago

It's so odd that the CI isn't finding the DANDI_API_KEY secret: https://github.com/catalystneuro/nwb-conversion-tools/runs/7342649698?check_suite_focus=true

I suppose it's because this is submitted from @JuliaSprenger's fork, but the actions appear to be going through the main repo...

EDIT: https://stackoverflow.com/a/58740879 would indicate this is indeed the case, where secrets are not available (besides GITHUB_TOKEN) when PR is submitted from a fork. Looking into a way to fix this.

CodyCBakerPhD commented 2 years ago

@JuliaSprenger Would you mind trying to swap these lines: https://github.com/catalystneuro/nwb-conversion-tools/blob/main/tests/test_internals/test_tools.py#L197-L200

to use @unittest.skipIf instead of pytest? That might fix the problem. It should be skipped no matter what if the secret token isn't available as an environment variable, but for some reason that does not appear to be the case right now.

CodyCBakerPhD commented 2 years ago

Wow, hmm... OK I had figured that it was possible to skip the entire test class that way, but apparently not...

@JuliaSprenger So sorry, but can you try skipping just that one function instead https://github.com/catalystneuro/nwb-conversion-tools/blob/main/tests/test_internals/test_tools.py#L209?

JuliaSprenger commented 2 years ago

Hi @CodyCBakerPhD Now only the issue of secret tokens not being available in PR triggered runs remains. It's a security feature to prevent secrets to be accessible by non-project administrators. A safe way on how to still have the key available is to use the pull_request_target trigger in combination with a manual label of the PR by an administrator, e.g. like I did it here: https://github.com/INT-NIT/DigLabTools/blob/main/.github/workflows/run_tests.yml

codecov[bot] commented 2 years ago

Codecov Report

Merging #597 (a3a5304) into main (85776d5) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #597   +/-   ##
=======================================
  Coverage   88.34%   88.34%           
=======================================
  Files          59       59           
  Lines        3218     3218           
=======================================
  Hits         2843     2843           
  Misses        375      375           
Flag Coverage Δ
unittests 88.34% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/nwb_conversion_tools/nwbconverter.py 97.40% <100.00%> (ø)
CodyCBakerPhD commented 2 years ago

A safe way on how to still have the key available is to use the pull_request_target trigger in combination with a manual label of the PR by an administrator

Looks like that is indeed working! Thanks!

JuliaSprenger commented 2 years ago

Hi @CodyCBakerPhD Now it works for you, but your secret Dandi key could be stolen if the tests trigger automatically for any new PR.