System tests for ActiveJobs #4583
Conversation
|
This is a comment acknowledging this PR. |
github-project-automation
bot
moved this from On Hold
to Merged/Closed
in PR Review Pipeline
github-project-automation
bot
moved this from Merged/Closed
to Awaiting Review
in PR Review Pipeline
johrstrom
added
component/activejobs
and removed
status/on hold
component/dashboard
labels
| require 'ood_core/job/adapters/slurm' | ||
|
|
||
| class ActiveJobsTest < ApplicationSystemTestCase | ||
| JavascriptWaitTime = 5 |
There was a problem hiding this comment.
Please use SNAKE_CASE_UPPER style for constants that aren't classes.
There was a problem hiding this comment.
🤦♂️That was a remnant from before I discovered assert_selector to wait for js action. It is unused and I will just remove it, but that is good to know going forward.
There was a problem hiding this comment.
OK I think that applies to variables as well. You also have DetailHeaders which should likely be detail_headers, MainbodySelect which is main_body_select and so on.
There was a problem hiding this comment.
Why wouldn't those be snake case upper as well? They are class constants like JavascriptWaitTime was.
Also I accidentally went ahead and converted them all to snake case upper, having not read your last comment carefully enough. Happy to change them to lower snake case though if that is what we want.
github-project-automation
bot
moved this from Awaiting Review
to Merged/Closed
in PR Review Pipeline
Contributes to #375, and relies upon the changes in #4576 and #4578. The first test exhaustively checks the row structure and extended info, allowing us to skip these checks in later tests, just confirming which jobs are shown using number of rows and assert_text. The main areas this test covers are