[AN-142] Adding cost centaur test #7685
Conversation
| } yield () | ||
| } | ||
|
|
||
| def validateNoCost(submittedWorkflow: SubmittedWorkflow): Test[Unit] = |
There was a problem hiding this comment.
maybe overkill, but I also added a test to verify that the cost is 0 when a workflow is call-cached
| @@ -0,0 +1,17 @@ | |||
| In order to run a Cromwell instance locally, there are the some prerequisites: | |||
There was a problem hiding this comment.
This is just a start from what I can remember, feel free to add/edit!
| @@ -0,0 +1,39 @@ | |||
| task getAverage { | |||
There was a problem hiding this comment.
Is this exactly the same as the existing cacheBetweenWF.wdl? If so it's fine to use that one in the new test rather than duplicating it.
There was a problem hiding this comment.
yeah I had issues with the cache ids not matching because I think they were pulling from the run ofcacheBetweenWf, I just had some fun creating a new WDL and pushed that up
centaur/src/main/resources/standardTestCases/recursive_imports/recursive_imports_cost.options
Outdated
Show resolved
Hide resolved
| } yield () | ||
| } | ||
|
|
||
| def validateNoCost(submittedWorkflow: SubmittedWorkflow): Test[Unit] = |
| } | ||
|
|
||
| /** | ||
| * Validate that the actual cost is within 10% of the expected cost |
There was a problem hiding this comment.
It might be more flexible and reliable to let the test define a min cost and max cost... I think this is OK as it, though, up to you.
There was a problem hiding this comment.
yeah I ran a bunch of test runs locally to validate that 10% seems to be a reasonable threshold (never hit or got very close to it). It would be also easy enough to increase that to 20% if we think it might cause flakiness. I can pretty much just as easily switch to a min/max strategy if we decide that's better though!
Some thoughts here, in my mind I guess the question becomes:
Are we testing that the cost is fairly consistent, in which case a threshold makes sense
OR
are we testing that a seemingly accurate cost is returned, then maybe a min/max makes sense?
There was a problem hiding this comment.
I think we're more testing for accuracy, rather than testing whether the cloud pricing has changed... but it's a subtle distinction.
| @@ -337,6 +357,15 @@ object TestFormulas extends StrictLogging { | |||
| case _ => Test.invalidTestDefinition("Configuration not supported by PapiUpgradeTest", workflowDefinition) | |||
| } | |||
|
|
|||
| def runSuccessfulWorkflowAndVerifyCost( | |||
There was a problem hiding this comment.
Did you consider updating the existing test format to optionally check cost (if it's provided in the test config)?
| * `sbt "centaur / IntegrationTest / test"` - compiles Centaur and runs all tests via sbt directly. Tests are expected to be in the `centaur/src/main/standardTestCases` directory. This can be changed by modifying `reference.conf`. | ||
| * `sbt "centaur / IntegrationTest / test"` - compiles Centaur and runs all tests via sbt directly. | ||
| * Can also just run `centaur/ IntegrationTest / test` from the sbt terminal. | ||
| Tests are expected to be in the `centaur/src/main/standardTestCases` directory. In order to run a set of specific tests, create a new subdirectory that contains the tests to run, then modify the path in the centaur `reference.conf`. |
There was a problem hiding this comment.
Does -n below in Tags cover this application?
There was a problem hiding this comment.
ok I swear I tried that early on and it wasn't working for me, but I just tried again and it did... I can remove this addition!
Description
Release Notes Confirmation
CHANGELOG.mdCHANGELOG.mdin this PRCHANGELOG.mdbecause it doesn't impact community usersTerra Release Notes