Closed tdowrick closed 3 years ago
In GitLab by @RolandGuichard on Dec 11, 2018, 17:31
added 1 commit
In GitLab by @RolandGuichard on Dec 11, 2018, 17:32
Needs a bit of renaming which I'll do next.
In GitLab by @RolandGuichard on Dec 12, 2018, 10:22
added 1 commit
In GitLab by @RolandGuichard on Dec 12, 2018, 10:28
test pass while running against Slicer 4.10 on my laptop.
In GitLab by @RolandGuichard on Dec 12, 2018, 10:35
assigned to @DavidPerez-Suarez
In GitLab by @RolandGuichard on Dec 12, 2018, 10:43
@DavidPerez-Suarez @AnastasisGeorgoulas could you please review ? Since there is no other way (so far) to notify you.
In GitLab by @AnastasisGeorgoulas on Dec 12, 2018, 16:11
Commented on skullbasenavigation/sbn/Testing/Python/CMakeLists.txt line 2
Just checking that this "empty" file is needed (I guess that CMake might want it there?)
In GitLab by @AnastasisGeorgoulas on Dec 12, 2018, 16:14
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 39
It feels like tf_name
and the other transform
variables should belong in a subclass, not the base TestNeedle
(as they're only used by certain test classes)
In GitLab by @AnastasisGeorgoulas on Dec 12, 2018, 16:15
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 119
Has this test been omitted because of a difficulty, or did it just slip through? :)
In GitLab by @AnastasisGeorgoulas on Dec 12, 2018, 16:16
Commented on CMakeLists.txt line 16
This should probably be something less user-specific!
In GitLab by @AnastasisGeorgoulas on Dec 12, 2018, 16:20
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 52
I'm not very familiar with unittest
, but maybe there is a better of passing the needle model to the other test cases - essentially, treat it as a fixture. That way, we could create the model only once, do the tests that only involve it, and then have the other test classes just use it for their own tests, without creating another one in their setup. Or maybe I'm misunderstanding how unittest
does fixtures.
In GitLab by @AnastasisGeorgoulas on Dec 12, 2018, 16:23
I also see the linting test fails, but the errors seem easily fixable, so that shouldn't be a problem. I'm happy to help with that (or any of the other comments) if it's an issue.
In GitLab by @RolandGuichard on Dec 12, 2018, 20:47
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 39
That was the original plan but it turned out certain tests need both. I would probably suggest to rename TestNeedle
to something less specific like TestModel
? Basically, it is used in unittest
as a fixture.
In GitLab by @RolandGuichard on Dec 12, 2018, 20:47
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 119
ah, well no. I forgot to delete it simply.
In GitLab by @RolandGuichard on Dec 12, 2018, 20:50
Commented on skullbasenavigation/sbn/Testing/Python/CMakeLists.txt line 2
That's taken from other extension examples. I was wondering too what is the real need for this, but it's pretty much everywhere.
In GitLab by @RolandGuichard on Dec 12, 2018, 20:52
Commented on CMakeLists.txt line 16
Again, this is standard to adding extensions in Slicer. Once the skull navigation module is completed, my understanding is that it ought to become a Slicer extension. Then this fields will be filled. All the CMake
machinery here is intended to run the tests.
In GitLab by @RolandGuichard on Dec 12, 2018, 20:53
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 52
Well, see above if it sheds more light.
In GitLab by @RolandGuichard on Dec 12, 2018, 20:54
Ah ? There are linting tests ? I've attempted to lint it with flake8
.
In GitLab by @RolandGuichard on Dec 14, 2018, 09:46
Commented on CMakeLists.txt line 16
But yeah in the end this one should be using environmental or CMake
variables.
In GitLab by @RolandGuichard on Dec 14, 2018, 09:47
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 119
changed this line in version 4 of the diff
In GitLab by @RolandGuichard on Dec 14, 2018, 09:47
added 1 commit
In GitLab by @AnastasisGeorgoulas on Dec 14, 2018, 14:00
Commented on CMakeLists.txt line 16
Yes, I meant the specific path used here.
In GitLab by @AnastasisGeorgoulas on Dec 14, 2018, 14:07
I think the tests are defined in the tox.ini
file, which has a linting test with pylint --rcfile=tests/pylintrc skullbasenavigation
. There are a few missing docstrings and some minor things in the pipeline output log (e.g. https://weisslab.cs.ucl.ac.uk/WEISS/SoftwareRepositories/skullbasenavigation/-/jobs/4196)
In GitLab by @AnastasisGeorgoulas on Dec 14, 2018, 14:16
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 39
I'm also getting some warnings (see below) when running this, which I think might be due to creating the transforms in all test cases rather than only the ones who use them.
Creating and testing a Needle model
Warning: In /Volumes/Dashboards/Stable/Slicer-4100/Libs/MRML/Core/vtkMRMLSubjectHierarchyNode.cxx, line 2577
vtkMRMLSubjectHierarchyNode (0x7fbd9b6b61f0): GetItemChildWithName: Multiple subject hierarchy item found with name 'test_create_transform' under item with ID 3. Returning first
I don't think it affects the results, but it would be good to avoid them if possible. We can probably refactor this!
In GitLab by @DavidPerez-Suarez on Dec 17, 2018, 14:12
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 1
Shouldn't the file be under the Testing/python
directory?
In GitLab by @DavidPerez-Suarez on Dec 17, 2018, 15:22
Regarding pylint errors:
In GitLab by @RolandGuichard on Jan 4, 2019, 17:57
Commented on CMakeLists.txt line 16
changed this line in version 5 of the diff
In GitLab by @RolandGuichard on Jan 4, 2019, 17:57
added 2 commits
In GitLab by @DavidPerez-Suarez on Jan 4, 2019, 18:50
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 39
changed this line in version 6 of the diff
In GitLab by @DavidPerez-Suarez on Jan 4, 2019, 18:50
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 52
changed this line in version 6 of the diff
In GitLab by @DavidPerez-Suarez on Jan 4, 2019, 18:50
Commented on skullbasenavigation/sbn/testing_slicer_functions.py line 1
changed this line in version 6 of the diff
In GitLab by @DavidPerez-Suarez on Jan 4, 2019, 18:50
added 1 commit
In GitLab by @DavidPerez-Suarez on Jan 14, 2019, 12:18
added 9 commits
In GitLab by @RolandGuichard on Jan 15, 2019, 14:45
Commented on skullbasenavigation/sbn/Testing/Python/CMakeLists.txt line 2
changed this line in version 8 of the diff
In GitLab by @RolandGuichard on Jan 15, 2019, 14:45
added 5 commits
In GitLab by @DavidPerez-Suarez on Feb 4, 2019, 17:55
added 59 commits
master
In GitLab by @ThomasDowrick on May 15, 2019, 13:54
merged
In GitLab by @RolandGuichard on Dec 10, 2018, 19:23
Merges python-test-runner -> master
Fixes #29