Skip to content

Chore: Fix broken tests #6458

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

Merged
rileyajones merged 2 commits into from
Jun 27, 2023
Merged

Chore: Fix broken tests #6458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de2fd9e
fix broken tests
rileyajones Jun 26, 2023
6be1522
add comment
rileyajones Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tensorboard/webapp/core/views/layout_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ describe('layout test', () => {
dispatchedActions.push(action);
});
store.overrideSelector(getSideBarWidthInPercent, 10);
// When the runs table is full screen the width is overridden to 100%.
store.overrideSelector(getRunsTableFullScreen, false);
Copy link
Member

@arcra arcra Jun 27, 2023

Choose a reason for hiding this comment

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

Is it obvious why this is necessary/desired? Is this a workaround to the issue we had, or doing something properly that wasn't done properly before?

Either way, would a comment help?

Copy link
Contributor Author

@rileyajones rileyajones Jun 27, 2023

Choose a reason for hiding this comment

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

This is a different failure that seems to only happen occasionally (I'm not sure why it doesn't always happen). While the primary issue relates back to #6442 this issue is more recent and relates back to #6451.

Copy link
Member

@arcra arcra Jun 27, 2023

Choose a reason for hiding this comment

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

It's not clear to me what this issue is, and the PR only says "fixes broken test".

I don't think that anybody else reading this line of code would understand why it's needed and know that it should not be removed.

Could you please update the PR description with what the issue(s) being fixed is (are), and add a comment in this line describing why it's useful/necessary?

Copy link
Contributor

@bmd3k bmd3k Jun 27, 2023

Choose a reason for hiding this comment

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

FWIW I looked at this line of code and understood what was happening. I don't think a comment is necessary.

Copy link
Member

@arcra arcra Jun 27, 2023

Choose a reason for hiding this comment

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

The state is initialized with this value already set to False, in this line: http://github.com/tensorflow/tensorboard/blob/master/tensorboard/webapp/core/testing/index.ts#L75

So my point was that while it's clear what the line does, it's not clear why it's needed. Someone reading these tests might think "this is the default value, this line shouldn't be necessary", and attempt to remove it, possibly resulting in flaky tests again.

I would have expected a comment like:
// This is the default when we initialize the state, so this line shouldn't be needed, but
// for some reason some tests seem to be using a state that is not the initial state (maybe link to issue).

Not a big deal, but that was my point. I'm sorry I didn't communicate my intention/expectation clearly.

Copy link
Contributor

@bmd3k bmd3k Jun 27, 2023

Choose a reason for hiding this comment

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

Actually, now that you expand on it, you're right, it doesn't make a great deal of sense. When I said I "understood what was happening" I was thinking back to the not-too-distant past where we were creating empty mock stores (instead of calls to provideMockTbStore()) and we had to override every single selector used by the component being tested.

I also agree the current comment doesn't explain the situation sufficiently.

});

afterEach(() => {
Expand Down
6 changes: 3 additions & 3 deletions tensorboard/webapp/runs/views/runs_table/runs_data_table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,15 @@ export class RunsDataTable {
}

getRunIds() {
return this.data.map((row) => row.id);
return (this.data || []).map((row) => row.id);
Copy link
Contributor

@bmd3k bmd3k Jun 27, 2023

Choose a reason for hiding this comment

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

I also find this a bit difficult to understand. this.data is a required property typed as TableData[] so the || [] part of this.data || [] should never be evaluated in practice.

Are you adding this just because of some deficiency in the test? If so, it would be better to fix the test.

}

allRowsSelected() {
return this.data.every((row) => row['selected']);
return (this.data || []).every((row) => row['selected']);
}

someRowsSelected() {
return this.data.some((row) => row['selected']);
return (this.data || []).some((row) => row['selected']);
}

onFilterKeyUp(event: KeyboardEvent) {
Expand Down