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

Auto generate release name #877

Merged
merged 2 commits into from
Dec 17, 2018

Conversation

andresmgot
Copy link
Contributor

Fixes: #485

Auto generate a random name for the release using moniker.

screenshot 2018-12-11 at 18 31 20

Copy link
Contributor

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Great!

expect(wrapper).toMatchSnapshot();
});

it("renders a release name by default", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could potentially mock Moniker to test that its generation is the value set and changes between mounts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd avoid all the sinon/stub integration just for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that? You do not need to use sinon but jest.

I think that the key here is if you care about testing that the value set as name changes and depends on an autogenerated value. Which is not being tested in your case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, well, in that case it's not necessary to mock the behavior just to check that the release name has a length bigger than 0 and that it changes.

@andresmgot andresmgot merged commit e1b2f19 into vmware-tanzu:master Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants