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.

Sign up for GitHub

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

Jump to bottom

Refs #31459 - remove snapshot testing in the job wizard #607

Merged
merged 3 commits into from Jul 12, 2021
Merged

Refs #31459 - remove snapshot testing in the job wizard #607

merged 3 commits into from Jul 12, 2021

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Jul 2, 2021

Moved the advanced step test to the advanced step folder and removed the job wizard snapshot test.

@MariaAga MariaAga changed the title Refs #31459 - remove snapshot testing Refs #31459 - remove snapshot testing in the job wizard Jul 2, 2021
@MariaAga MariaAga force-pushed the remove-job-wizard-snapshots branch from a938db4 to da68c9a Compare July 2, 2021 09:26
@MariaAga
Copy link
Member Author

MariaAga commented Jul 2, 2021

missing the categories will remove the snapshots there as well

@MariaAga MariaAga force-pushed the remove-job-wizard-snapshots branch 2 times, most recently from 45e27c6 to 038a5f5 Compare July 7, 2021 16:04
@MariaAga
Copy link
Member Author

MariaAga commented Jul 7, 2021

Moved some code into test setups as we discussed @LaViro

@MariaAga MariaAga force-pushed the remove-job-wizard-snapshots branch from 038a5f5 to b2201e2 Compare July 7, 2021 16:10
@ezr-ondrej
Copy link
Member

Snapshots are failing xD :)

@MariaAga
Copy link
Member Author

MariaAga commented Jul 7, 2021

At least the snapshots make it easier for me to find them and delete them 😈

@MariaAga MariaAga force-pushed the remove-job-wizard-snapshots branch from b2201e2 to fec5043 Compare July 8, 2021 13:54
MariaAga
MariaAga commented Jul 8, 2021
View reviewed changes
webpack/JobWizard/steps/form/Formatter.js
key={name}
isRequired={required}
label={name}
fieldId={name}
options={options}
labelText={labelText}
Copy link
Member Author

Choose a reason for hiding this comment

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

labelText is not a real prop

@MariaAga MariaAga force-pushed the remove-job-wizard-snapshots branch 2 times, most recently from 7518a8b to 8078dee Compare July 8, 2021 14:06
ezr-ondrej
ezr-ondrej reviewed Jul 10, 2021
View reviewed changes
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Could you separate the Select field fix in separate PR or at least commit pls?

Also sidenote: why do we have SelectField here, when there is most certainly a SelectField in Foreman core (it's quite new, so that would be my guess, but it would be worth considering using it), I'll not block the PR on that tho, so up to you :)

@MariaAga MariaAga force-pushed the remove-job-wizard-snapshots branch from 8078dee to fa65abd Compare July 12, 2021 08:25
@MariaAga MariaAga mentioned this pull request Jul 12, 2021
Merged
@MariaAga
Copy link
Member Author

Separated into 2 commits.
There is no pf4 select field in core from what I saw

Ron-Lavi
Ron-Lavi reviewed Jul 12, 2021
View reviewed changes
Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Overall looks good to me,
just a couple of inline comments

webpack/JobWizard/__tests__/integration.test.js Outdated
jest.spyOn(selectors, 'selectJobCategoriesStatus');
const store = testSetup(selectors, api);

selectors.selectJobTemplate.mockImplementation(() => {});
jest.spyOn(selectors, 'selectEffectiveUser');
jest.spyOn(selectors, 'selectTemplateInputs');
jest.spyOn(selectors, 'selectAdvancedTemplateInputs');
Copy link
Member

Choose a reason for hiding this comment

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

should these selectors move to the fixtures file as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

those are leftovers so I removed them now :)

webpack/JobWizard/steps/AdvancedFields/__tests__/AdvancedFields.test.js
Comment on lines +17 to +30
jest.spyOn(selectors, 'selectEffectiveUser');
jest.spyOn(selectors, 'selectTemplateInputs');
jest.spyOn(selectors, 'selectAdvancedTemplateInputs');

selectors.selectEffectiveUser.mockImplementation(
() => jobTemplate.effective_user
);
selectors.selectTemplateInputs.mockImplementation(
() => jobTemplate.template_inputs
);

selectors.selectAdvancedTemplateInputs.mockImplementation(
() => jobTemplate.advanced_template_inputs
);
Copy link
Member

Choose a reason for hiding this comment

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

should also move to fixtures?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think they should as they are only relevant to the advanced step

webpack/JobWizard/steps/AdvancedFields/__tests__/__snapshots__/AdvancedFields.test.js.snap
</Form>
</AdvancedFields>
</Provider>
`;
Copy link
Member

Choose a reason for hiding this comment

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

Goodbye 422 lines 🥇

webpack/JobWizard/steps/CategoryAndTemplate/CategoryAndTemplate.test.js Outdated
Comment on lines 11 to 16
jest.spyOn(selectors, 'selectJobTemplate');
jest.spyOn(selectors, 'selectJobTemplates');
jest.spyOn(selectors, 'selectJobCategories');
jest.spyOn(selectors, 'selectCategoryError');
jest.spyOn(selectors, 'selectAllTemplatesError');
jest.spyOn(selectors, 'selectTemplateError');
Copy link
Member

Choose a reason for hiding this comment

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

how about those?

Copy link
Member Author

Choose a reason for hiding this comment

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

left only select___Error as they are the only relevant ones for the tst

Ron-Lavi
Ron-Lavi reviewed Jul 12, 2021
View reviewed changes
.../react_app/components/TargetingHosts/__tests__/__snapshots__/TargetingHostsPage.test.js.snap
@@ -27,6 +27,7 @@ exports[`TargetingHostsPage renders 1`] = `
"controller": "hosts",
}
}
onChange={[Function]}
Copy link
Member

Choose a reason for hiding this comment

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

Another snapshot appeared 😮

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a searchbar mock ( instead of the foreman one) that is just <input onchange={onchange}> so it affected other components

Ron-Lavi
Ron-Lavi approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga !

@ezr-ondrej
Copy link
Member

Ruby failures unrelated, thanks @MariaAga and @LaViro! 👍

@ezr-ondrej ezr-ondrej merged commit 57bc33d into theforeman:master Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezr-ondrej ezr-ondrej ezr-ondrej left review comments

@Ron-Lavi Ron-Lavi Ron-Lavi approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
4 participants
@MariaAga @ezr-ondrej @Ron-Lavi @theforeman-bot