Skip to content

Conversation

debs-obrien
Copy link
Contributor

No description provided.

@@ -168,13 +168,20 @@ test.describe('Item', () => {

const firstTodo = page.getByTestId('todo-item').nth(0);
const secondTodo = page.getByTestId('todo-item').nth(1);
await firstTodo.getByRole('checkbox').check();
const firstTodoCheck = firstTodo.getByRole('checkbox');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const firstTodoCheck = firstTodo.getByRole('checkbox');
const firstTodoCheckbox = firstTodo.getByRole('checkbox');

don't call it Check, call it Checkbox

const secondTodoCheck = secondTodo.getByRole('checkbox');

await firstTodoCheck.check();
await expect(firstTodoCheck).toBeChecked();
Copy link
Member

@mxschmitt mxschmitt Oct 31, 2022

Choose a reason for hiding this comment

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

I feel like this assertion does not make a lot of sense, since a checkbox is always checked if you check it. Isn't asserting for the completed class already enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok perhaps. no idea to be honest.

@debs-obrien debs-obrien requested a review from mxschmitt October 31, 2022 19:54
@@ -168,12 +168,15 @@ test.describe('Item', () => {

const firstTodo = page.getByTestId('todo-item').nth(0);
const secondTodo = page.getByTestId('todo-item').nth(1);
await firstTodo.getByRole('checkbox').check();
const firstTodoCheckbox = firstTodo.getByRole('checkbox');
const secondTodoCheckbox = secondTodo.getByRole('checkbox');
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused?

@@ -317,8 +320,10 @@ test.describe('Persistence', () => {
}

const todoItems = page.getByTestId('todo-item');
await todoItems.nth(0).getByRole('checkbox').check();
const firstTodoCheck = todoItems.nth(0).getByRole('checkbox')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const firstTodoCheck = todoItems.nth(0).getByRole('checkbox')
const firstTodoCheck = todoItems.nth(0).getByRole('checkbox');

@@ -167,12 +167,15 @@ test.describe('Item', () => {

const firstTodo = page.getByTestId('todo-item').nth(0);
const secondTodo = page.getByTestId('todo-item').nth(1);
await firstTodo.getByRole('checkbox').check();
const firstTodoCheckbox = firstTodo.getByRole('checkbox');
const secondTodoCheckbox = secondTodo.getByRole('checkbox');
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused!

@@ -316,8 +319,10 @@ test.describe('Persistence', () => {
}

const todoItems = page.getByTestId('todo-item');
await todoItems.nth(0).getByRole('checkbox').check();
const firstTodoCheck = todoItems.nth(0).getByRole('checkbox')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const firstTodoCheck = todoItems.nth(0).getByRole('checkbox')
const firstTodoCheck = todoItems.nth(0).getByRole('checkbox');

@debs-obrien debs-obrien merged commit a0716dc into microsoft:main Nov 2, 2022
@debs-obrien debs-obrien deleted the check-checked branch November 2, 2022 12:55
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.

3 participants