fix(html): display error when test cannot be displayed #35661
Conversation
This comment has been minimized.
This comment has been minimized.
| width: 100%; | ||
| } | ||
|
|
||
| .error { |
There was a problem hiding this comment.
global style with the name like this would be a bad idea.
There was a problem hiding this comment.
CSS Modules would solve it - so it would not conflict with other components. Afaik a common best-practise nowadays and built-in in Vite. Maybe a good time to try?
There was a problem hiding this comment.
I'd love to move to CSS modules. I assumed in this case that the global error would be correct for its use.
| {!!report && <TestCaseViewLoader report={report} tests={filteredTests.tests} testIdToFileIdMap={testIdToFileIdMap} />} | ||
| {!!report ? | ||
| <TestCaseViewLoader report={report} tests={filteredTests.tests} testIdToFileIdMap={testIdToFileIdMap} /> : | ||
| <div className='error'>Report data could not be found</div>} |
There was a problem hiding this comment.
What does this mean? What do I do?
There was a problem hiding this comment.
A fatal error occurred with loading the report and there's likely no recovery. The best thing the user can do is reload. We currently show a blank screen if this happens.
There was a problem hiding this comment.
How is Report data could not be found better than blank screen?
There was a problem hiding this comment.
Generally it is better to communicate to the user that something broke, rather than doing nothing. Technically a blank screen implicitly communicates something broke, but it's still a poor experience.
Any place where an error can occur but is not handled (generally explicitly) is a bug. This is no different. I agree that the error message could be better though.
| type: 'test', | ||
| test: TestCase, | ||
| } | { | ||
| type: 'error', |
There was a problem hiding this comment.
That would not be a very useful state. How about a placeholder that suggests possible recovery when the report is empty?
| }> = ({ report, testIdToFileIdMap, tests }) => { | ||
| const searchParams = React.useContext(SearchParamsContext); | ||
| const [test, setTest] = React.useState<TestCase | undefined>(); | ||
| const [test, setTest] = React.useState<TestState>({ type: 'loading' }); |
There was a problem hiding this comment.
It is immediately harder to read. Let's keep things more direct. We used to have a test: TestCase, understand that. We can handle undefined test case, we can detect if the test is not the the map and handle that. Where is the state coming from?
|
Is there an issue? I'd like to take a stab at it, but I'd like to know how I can repro / test it. |
|
No issue. I don't have a repro for the no report case; not sure if/when this could occur. Would require a malformed base64 string/zip. Repro for the test information is simple. Change the |
Test results for "tests 1"12 failed 4 flaky39045 passed, 799 skipped Merge workflow run. |
pavelfeldman
left a comment
pavelfeldman
left a comment
There was a problem hiding this comment.
This touches a bunch of code all over the place. I'll land my change instead.
3 participants
Display a simple error message when the user navigates to an invalid URL (for example, reloading between report generations).