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 brittle unit-test. #1172

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Remove brittle unit-test. #1172

merged 1 commit into from
Feb 26, 2024

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Feb 23, 2024

The removed test only checks if certain substrings are present. The golden testing automatically adds these tests for any usecase. Therefore, this test is not needed anymore.

The usecase for implementing non-specific current, will add a version of a passive current; and test the values against the analytical reference.

Ions are already covered by the ionic usecase.

Array variables are covered by the cnexp_array usecase.

Parameters coved by the parameters usecase.

Simple ODEs are covered by cnexp_* usecases.

The removed test only checks if certain substrings are present. The
golden testing automatically adds these tests for any usecase.
Therefore, this test is not needed anymore.

The usecase for implementing non-specific current, will add a version
of a passive current; and test the values against the analytical
reference.

Ions are already covered by the `ionic` usecase.

Array variables are covered by the `cnexp_array` usecase.

Parameters coved by the `parameters` usecase.

Simple ODEs are covered by `cnexp_*` usecases.
@1uc
Copy link
Collaborator Author

1uc commented Feb 23, 2024

While developing NRN support in NMODL, these tests are very annoying. I can never find the difference; and it's completely redundant with #1131. The CNRN variety can be revisited ater, since code generated for CNRN shouldn't be changing right now, it doesn't cause any issues.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.16%. Comparing base (c0eb1ac) to head (cb71564).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1172      +/-   ##
==========================================
- Coverage   88.38%   87.16%   -1.23%     
==========================================
  Files         176      176              
  Lines       12946    12890      -56     
==========================================
- Hits        11442    11235     -207     
- Misses       1504     1655     +151     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #195923 (:white_check_mark:) have been uploaded here!

Status and direct links:

@1uc 1uc marked this pull request as ready for review February 26, 2024 09:43
Copy link
Contributor

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Sounds reasonable!

@1uc 1uc merged commit 2ea7f36 into master Feb 26, 2024
21 checks passed
@1uc 1uc deleted the 1uc/remove-brittle-test branch February 26, 2024 11:58
ohm314 pushed a commit that referenced this pull request May 21, 2024
The removed test only checks if certain substrings are present. The
golden testing automatically adds these tests for any usecase.
Therefore, this test is not needed anymore.

The usecase for implementing non-specific current, will add a version
of a passive current; and test the values against the analytical
reference.

Ions are already covered by the `ionic` usecase.

Array variables are covered by the `cnexp_array` usecase.

Parameters coved by the `parameters` usecase.

Simple ODEs are covered by `cnexp_*` usecases.
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

3 participants