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

Implement getColumnDescription getter #289

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

surchs
Copy link
Contributor

@surchs surchs commented Dec 21, 2022

There is a bit more in this PR than just the normal refactor. The reason is that adding a new test for the
column-linking-component somehow broke the existing component test in a way I didn't understand.
The original column-linking-component test passed when forced to run alone it.only, but could not wrap the
commit function on the store object in a cy.spy when it was run second after the new test.

I think this may be because running the first test somehow changes the mocked store object in a way
that affects subsequent test. I was not able to debug this though, the store object looked normal to me.
As a workaround, I have moved the mocked store into a beforeEach statement to make sure every test
gets passed the same store object.

There may be better ways of doing this, and I think we should research these separately. I made #288 to track this.

Closes #253

@surchs surchs changed the base branch from master to dev_components_talk_to_store_directly December 21, 2022 20:08
Copy link
Contributor

@jarmoza jarmoza left a comment

Choose a reason for hiding this comment

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

@surchs Other than discussion of the best way to provide unique instances of the mock store in #288, this PR looks good to me. Check out my suggestion in #288. Otherwise, I think this is okay to go – the beforeEach solution is reasonable and with precedent for testing.

@surchs
Copy link
Contributor Author

surchs commented Dec 21, 2022

Thanks @jarmoza. I like your comments on #288, but I would wait until we have a decision on this before I implement it. Would you mind still approving the PR - I can technically merge without, but I think we should probably not make a habit of it.

@surchs surchs requested a review from jarmoza December 22, 2022 15:42
Copy link
Contributor

@jarmoza jarmoza left a comment

Choose a reason for hiding this comment

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

Got it, @surchs. Approved.

Copy link
Contributor

@rmanaem rmanaem left a comment

Choose a reason for hiding this comment

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

Good to go!

I ran into a weird problem where I was not able to wrap the commit
method on the store object in a cypress spy, only when the test
ran after another test.

I assume that running the other test somehow changed the store object.
Re-initializing the mocked store object with beforeEach fixed this.
@surchs surchs merged commit f066860 into dev_components_talk_to_store_directly Dec 22, 2022
@surchs surchs deleted the surchs/issue253 branch December 22, 2022 16:24
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.

create getter to get column description
3 participants