Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recurring run details tests #202

Merged
merged 5 commits into from
Nov 15, 2018

Conversation

yebrahim
Copy link
Contributor

@yebrahim yebrahim commented Nov 11, 2018

  • Fixes a bug in RecurringRunDetails where an undefined enabled field shows both Enable and Disable buttons as active.
  • Adds comprehensive test suite for RecurringRunDetails.

This change is Reviewable

@coveralls
Copy link

coveralls commented Nov 11, 2018

Pull Request Test Coverage Report for Build 342

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 20 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+10.6%) to 78.152%

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%
src/components/Router.tsx 13 49.18%
Totals Coverage Status
Change from base Build 220: 10.6%
Covered Lines: 1775
Relevant Lines: 2193

💛 - Coveralls

@vicaire vicaire removed their request for review November 12, 2018 21:15
},
} as ApiJob;

historyPushSpy.mockClear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this big block of calls should be reducible using mockReset()
https://jestjs.io/docs/en/mock-function-api.html#mockfnmockreset
"mockReset() does everything that mockFn.mockClear() does, and also removes any mocked return values or implementations.",

-- unless you're just looking to have dummy functions for many of them, but it still seems like calling reset is preferable to clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does mockReset reduce the calls here?
It seems to me like you either do mockReset followed by a mockImplementation call, or you just do mockClear no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on what your intent is with the mockImplementation() calls. If you're just trying to remove whatever other mock implementation there was, you should use mockReset, if you actually want there to be an empty implementation, then calling clear + implement is right.

In the cases where you're calling mockClear without mockImplementation though, it seems like reset should be preferable because that way you make sure you remove whatever the previous mocked implementation was

frontend/src/pages/RecurringRunDetails.test.tsx Outdated Show resolved Hide resolved
frontend/src/pages/RecurringRunDetails.test.tsx Outdated Show resolved Hide resolved
frontend/src/pages/RecurringRunDetails.test.tsx Outdated Show resolved Hide resolved
@rileyjbauer
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

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:

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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

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:

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

@rileyjbauer
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 74270cb into master Nov 15, 2018
@yebrahim yebrahim deleted the yebrahim/recurring-run-details-tests branch November 15, 2018 22:07
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
m-rafeeq pushed a commit to m-rafeeq/data-science-pipelines that referenced this pull request Jan 21, 2025
update dockerfiles to use ubi images and update go to 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants