Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 36.18% 35.68% -0.50%
==========================================
Files 14 14
Lines 702 723 +21
Branches 66 71 +5
==========================================
+ Hits 254 258 +4
- Misses 439 456 +17
Partials 9 9 ☔ View full report in Codecov by Sentry. |
MichelleGower
left a comment
MichelleGower
left a comment
There was a problem hiding this comment.
Some suggestions. Underscore doesn't have to be added, but strongly recommend increasing the unit testing. Merge approved.
| if anodeset is None: | ||
| jobname = f"glide_{auser}" | ||
| else: | ||
| jobname = f"{anodeset}glide_{auser}" |
There was a problem hiding this comment.
I would have put an underscore between anodeset and "glide".
| if anodeset is None: | ||
| full_constraint += " && (JobNodeset is None)" | ||
| else: | ||
| full_constraint += f" && {jnodeset}" |
There was a problem hiding this comment.
Since jnodeset isn't used until this line, I would either move the assignment (from line 349) to this else block or just eliminate jnodeset variable and put value directly into the full_constraint string (similar to the None above).
| if anodeset is None: | ||
| jobname = f"{auser}_{shash}" | ||
| else: | ||
| jobname = f"{anodeset}{auser}_{shash}" |
There was a problem hiding this comment.
Similar comment about putting an underscore after anodeset
| if anodeset is None: | ||
| jobname = f"glide_{auser}" | ||
| else: | ||
| jobname = f"{anodeset}glide_{auser}" |
There was a problem hiding this comment.
Same underscore comment.
Also wondering if could simplify all the code if anodeset gets set to empty string instead of None.
| self.assertEqual(minautocpus, 15) | ||
| self.assertEqual(cpus, 12) | ||
| self.assertEqual(nodes, 64) | ||
| self.assertEqual(nodeset, "DRP") |
There was a problem hiding this comment.
I think making this test2 (leaving test1 as is without nodeset given but adding a check that it is None) would improve the test coverage.
There was a problem hiding this comment.
I don't think it makes sense to go this test2 route. In order to assign --nodeset in the test, and check it vis getNodeset at the end, the entire coding/setup that is there in test1 has to be duplicated in test2. It will be the same test with a different line or two, that can be just as easily checked within test1 itself. Even if it makes the robot print a 'better number' this is no way to spend our time/resources.
2 participants
No description provided.