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

test: migrate markdown editor to component tests [TOL-1355] #1553

Merged
merged 5 commits into from
Feb 19, 2024

Conversation

MayaGillilan
Copy link
Contributor

No description provided.

@MayaGillilan MayaGillilan requested a review from a team as a code owner November 28, 2023 13:40
@github-actions github-actions bot added the tests label Nov 28, 2023
Copy link

Marking pull request as stale since there was no activity for 30 days

@github-actions github-actions bot added the stale Used to mark when there was no activity for a set period of time label Dec 29, 2023
@MayaGillilan MayaGillilan changed the title test: migrate markdown editor to component tests test: migrate markdown editor to component tests [TOL-1355] Feb 13, 2024
@github-actions github-actions bot removed the stale Used to mark when there was no activity for a set period of time label Feb 14, 2024
@MayaGillilan MayaGillilan merged commit 5a4ab39 into master Feb 19, 2024
14 checks passed
@MayaGillilan MayaGillilan deleted the test/migrate-markdown-editor branch February 19, 2024 09:01
Copy link
Contributor

@colomolo colomolo left a comment

Choose a reason for hiding this comment

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

Sorry, a bit of late review.

it('should have correct title', () => {
renderMarkdownEditor();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this render to beforeEach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could for some contexts, but it has some props so that means more code to change if we later want to add props for some reason

it('should have correct title', () => {
renderMarkdownEditor();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Can we use beforeEach?

};

export const clearAll = (): void => {
//Using extra select all because of flakiness with a single clear
Copy link
Contributor

Choose a reason for hiding this comment

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

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment looks like excessive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, I didn't want to assume it would be obvious since clear() also does .type('{selectall}'), but we could remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, then makes sense! Maybe mention it there then for those who didn't know that (me)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants