Adds NewRun tests #242

rileyjbauer · 2018-11-13T23:02:47Z

With this PR, the NewRun file has nearly complete coverage (by lines).

A little cleanup is done within the NewExperiment test as well.

The _prepareFormFromClone() function in NewRun did not actually change except that the wrapping if-block was changed to just return on the negative condition.

We no longer attempt to change the query params in handleChange when the pipelineID changes as it only would be called during a clone flow, and there's no real reason to do it.

This change is

… in Utils.ts

coveralls · 2018-11-13T23:07:01Z

Pull Request Test Coverage Report for Build 337

0 of 0 changed or added relevant lines in 0 files are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage increased (+7.5%) to 74.993%

Files with Coverage Reduction	New Missed Lines	%
src/lib/Utils.ts	1	90.16%
src/pages/NewRun.tsx	1	96.37%
src/pages/PipelineSelector.tsx	5	56.1%

Totals
Change from base Build 279:	7.5%
Covered Lines:	1704
Relevant Lines:	2192

💛 - Coveralls

coveralls · 2018-11-13T23:07:01Z

Pull Request Test Coverage Report for Build 310

0 of 0 changed or added relevant lines in 0 files are covered.
9 unchanged lines in 3 files lost coverage.
Overall coverage increased (+7.6%) to 75.093%

Files with Coverage Reduction	New Missed Lines	%
src/lib/Utils.ts	1	90.16%
src/pages/NewRun.tsx	1	96.34%
src/pages/PipelineSelector.tsx	7	63.41%

Totals
Change from base Build 279:	7.6%
Covered Lines:	1702
Relevant Lines:	2190

💛 - Coveralls

yebrahim

This is excellent coverage! Thanks!

yebrahim · 2018-11-14T07:23:04Z

frontend/src/pages/NewRun.tsx

-              {pipeline && (pipeline.parameters || []).map((param, i) =>
-                <TextField key={i} variant='outlined' label={param.name} value={param.value || ''}
+              {pipeline.parameters.map((param, i) =>
+                <TextField id={`newRunPipelineParam${i}`} key={i} variant='outlined'


It's a little excessive to have an id per field only to enable tests. They can use the parent's selector plus an nth-child kind of selector instead?

Is there any real harm here though? I'll take full responsibility if there's ever a collision with newRunPipelineParamX, and IDs make it so much more deterministic

No harm, just a matter of style. :) I'm ok leaving these around.

yebrahim · 2018-11-14T07:24:39Z

frontend/src/pages/NewRun.tsx

@@ -225,7 +230,8 @@ class NewRun extends Page<{}, NewRunState> {
          experimentId = RunUtils.getFirstExperimentReferenceId(originalRun.run);
        }
      } catch (err) {
-        await this.showPageError(`Error: failed to get original run: ${originalRunId}.`, err);
+        await this.showPageError(`Error: failed to retrieve original run: ${originalRunId}.`, err);
+        logger.error(`Failed to retrieve original run: ${originalRunId}`, err);


Console log doesn't really contain any more details in this case.

Same below. I think it's only useful to console log the error if we don't have a way of showing to the user yet, so we can ask them to get more details from the console while filing an issue.

I think it does. When we call showPageError, we end up only using the error's message, but when we log it with logger, we include the full stacktrace as well

frontend/src/pages/NewRun.tsx

yebrahim · 2018-11-14T07:35:59Z

frontend/src/pages/PipelineSelector.tsx

@@ -113,6 +115,9 @@ class PipelineSelector extends React.Component<PipelineSelectorProps, PipelineSe
      logger.error('Could not get list of pipelines', errorMessage);
    }

+    // Warnings are showing up here in tests when the NewRun component is unmounted before being
+    // closed, saying setState is being called after the component is unmounted.
+    // The warning doesn't show up when actually using the app though.


Use setStateSafe? Here and elsewhere.

Doing some reading, it sounds like that's not how we should be handling this, and that mostly just hides the warning, see: https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html

Yes, we should be looking at canceling promises on unmount, but that's an expensive fix. I don't see a lot of down sides to this, other than just be a little ugly.

Also forgot to mention, the setStateSafe function is only available for Page components, though i could emulate it here fairly easily

Yes, we probably should. I'll open an issue for properly fixing this, although we shouldn't really be holding our breath to do it. It might not be the best way to handle this, but it's not a bad fix IMO.

Do you want me to do add that code to this component for this PR then?

Up to you, it's probably not a lot of code so I'd do it, but it's fine to do this separately too.

frontend/src/pages/NewRun.test.tsx

yebrahim

Suite! Thanks. :)
/lgtm
/approve

yebrahim · 2018-11-15T00:22:18Z

frontend/src/pages/NewRun.tsx

-        await this.showPageError(`Error: failed to retrieve pipeline with ID: ${pipelineId}.`, err);
-        logger.error(`Error: failed to retrieve pipeline with ID: ${pipelineId}`, err);
+        this.setState({ pipelineSelectorOpen: false }, async () => {
+          const errorMessage = await errorToMessage(err);


Separately, I wonder if errorToMessage should be called within showErrorDialog too.

yes, that probably makes sense. we do it already for page errors

yebrahim · 2018-11-15T00:24:05Z

frontend/src/pages/NewRun.test.tsx

@@ -38,19 +37,23 @@ class TestNewRun extends NewRun {
 }

 describe('NewRun', () => {
+  const consoleErrorSpy = jest.spyOn(console, 'error');


Now looking at this, I'm wondering how this works. I can see the same thing appears in other suites, but if we're not resetting the spy, how come spying on a spy doesn't fail?

Aren't I resetting it below in the beforeEach ?

Yes, but that's beforeEach, which means when this suite is done, the method will be mocked. I'm not sure what's happening here, perhaps it's getting reset automatically or something.

yebrahim · 2018-11-15T00:25:14Z

frontend/src/pages/NewRun.test.tsx

  const createJobSpy = jest.spyOn(Apis.jobServiceApi, 'createJob');
  const createRunSpy = jest.spyOn(Apis.runServiceApi, 'createRun');
  const getExperimentSpy = jest.spyOn(Apis.experimentServiceApi, 'getExperiment');
  const getPipelineSpy = jest.spyOn(Apis.pipelineServiceApi, 'getPipeline');
  const getRunSpy = jest.spyOn(Apis.runServiceApi, 'getRun');
  const historyPushSpy = jest.fn();
  const historyReplaceSpy = jest.fn();
-  const listPipelinesSpy = jest.spyOn(Apis.pipelineServiceApi, 'listPipelines');


How come we don't need this? Shouldn't we get an error that fetch has failed when the pipeline selector opens? Or do we never use mount in tests below, never actually rendering the selector?

Not sure. There are some tests where we fully mount the tree, and interact with the selector, but it doesn't seem to be a problem. perhaps it's related to the peculiarities of Dialog that was preventing us from testing the "dismiss" paths?

k8s-ci-robot · 2018-11-15T00:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yebrahim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [yebrahim]
~~test/frontend-integration-test/OWNERS~~ [yebrahim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2018-11-15T00:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yebrahim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [yebrahim]
~~test/frontend-integration-test/OWNERS~~ [yebrahim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) * Adding disallowed list * Change to list

rileyjbauer added 2 commits November 13, 2018 14:41

Adds tests for the NewRun page

455a801

Add more tests

6268f20

rileyjbauer added area/frontend area/testing labels Nov 13, 2018

k8s-ci-robot requested review from qimingj, yebrahim and IronPan November 13, 2018 23:02

k8s-ci-robot added size/XXL labels Nov 13, 2018

Adds many more tests, cleans-up PipelineSelector a bit, and fixes bug…

c1acc7c

… in Utils.ts

rileyjbauer force-pushed the new-run-tests branch from f56e4ab to c1acc7c Compare November 13, 2018 23:03

Update create run button ID in e2e test

cabd68a

yebrahim reviewed Nov 14, 2018

View reviewed changes

Test clean-up and PR comments

8638a06

yebrahim approved these changes Nov 15, 2018

View reviewed changes

k8s-ci-robot assigned yebrahim Nov 15, 2018

k8s-ci-robot added the lgtm label Nov 15, 2018

k8s-ci-robot added the approved label Nov 15, 2018

k8s-ci-robot merged commit 4550513 into master Nov 15, 2018

IronPan deleted the new-run-tests branch July 31, 2019 06:28

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Allow all annotations to be set except the disallowed list (kubeflow#242

df9e05b

) * Adding disallowed list * Change to list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds NewRun tests #242

Adds NewRun tests #242

rileyjbauer commented Nov 13, 2018 •

edited by jlewi

Loading

coveralls commented Nov 13, 2018 •

edited

Loading

coveralls commented Nov 13, 2018

yebrahim left a comment

yebrahim Nov 14, 2018

rileyjbauer Nov 14, 2018

yebrahim Nov 14, 2018

yebrahim Nov 14, 2018

yebrahim Nov 14, 2018

rileyjbauer Nov 14, 2018

yebrahim Nov 14, 2018

rileyjbauer Nov 14, 2018 •

edited

Loading

yebrahim Nov 14, 2018

rileyjbauer Nov 14, 2018

yebrahim Nov 15, 2018

rileyjbauer Nov 15, 2018

yebrahim Nov 15, 2018

yebrahim left a comment

yebrahim Nov 15, 2018

rileyjbauer Nov 15, 2018

yebrahim Nov 15, 2018

rileyjbauer Nov 15, 2018

yebrahim Nov 15, 2018

yebrahim Nov 15, 2018

rileyjbauer Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

Adds NewRun tests #242

Adds NewRun tests #242

Conversation

rileyjbauer commented Nov 13, 2018 • edited by jlewi Loading

coveralls commented Nov 13, 2018 • edited Loading

Pull Request Test Coverage Report for Build 337

💛 - Coveralls

coveralls commented Nov 13, 2018

Pull Request Test Coverage Report for Build 310

💛 - Coveralls

yebrahim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyjbauer Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebrahim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

rileyjbauer commented Nov 13, 2018 •

edited by jlewi

Loading

coveralls commented Nov 13, 2018 •

edited

Loading

rileyjbauer Nov 14, 2018 •

edited

Loading