Chore: Fix broken tests #6458
Chore: Fix broken tests #6458
Conversation
| dispatchedActions.push(action); | ||
| }); | ||
| store.overrideSelector(getSideBarWidthInPercent, 10); | ||
| store.overrideSelector(getRunsTableFullScreen, false); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
FWIW I looked at this line of code and understood what was happening. I don't think a comment is necessary.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| dispatchedActions.push(action); | ||
| }); | ||
| store.overrideSelector(getSideBarWidthInPercent, 10); | ||
| store.overrideSelector(getRunsTableFullScreen, false); |
There was a problem hiding this comment.
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?
|
|
||
| getRunIds() { | ||
| return this.data.map((row) => row.id); | ||
| return (this.data || []).map((row) => row.id); |
There was a problem hiding this comment.
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.
| dispatchedActions.push(action); | ||
| }); | ||
| store.overrideSelector(getSideBarWidthInPercent, 10); | ||
| store.overrideSelector(getRunsTableFullScreen, false); |
There was a problem hiding this comment.
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.
## Motivation for features / changes In #6458 I fixed a couple of a tests which should have been failing but were sporadically passing. In this PR I believe I have fixed the underlying cause of the flakiness. Thanks #6266 for pointing the way. This broke nightly last night https://github.com/tensorflow/tensorboard/actions/runs/5388986210/jobs/9782358616
3 participants
Motivation for features / changes
Somehow some tests got broken on master and caused nightly to fail
https://github.com/tensorflow/tensorboard/actions/runs/5377348327/jobs/9755711856#step:11:2529