DEVPROD-9626: Allow rendering command sections outside of function sections #325
Conversation
…e are auto filtered
apps/parsley/src/components/LogRow/SubsectionHeader/SubsectionHeader.test.tsx
Outdated
Show resolved
Hide resolved
| const isFuncOpen = | ||
| (sectionState[func?.functionID]?.isOpen || | ||
| func?.containsTopLevelCommand) ?? | ||
| false; |
There was a problem hiding this comment.
if there a reason we can't just automatically set the section to be open during processing if the function contains top level command?
There was a problem hiding this comment.
I rather handle this during processedLogLine construction (the render prop) so I can treat functions that contain top level commands the same as any other function when it comes to toggling all sections or any other open/close state logic we might add in the future.
There was a problem hiding this comment.
I think it seems confusing because if the command is visible, doesn't that mean that the invisible parent function is technically open all of the time? but the SectionState says that the function is not open -- wouldn't that deviate logically from the pattern that we have now?
I didn't really investigate that hard but it seems like it'd might be possible to add isOpen: containsTopLevelCommand || isOpen in the sections code somewhere
There was a problem hiding this comment.
I refactored the code so the open state that's pushed to ProcessedLogLines is not related to containsTopLevelCommand. I also made this refactor ticket: https://jira.mongodb.org/browse/DEVPROD-10149
There was a problem hiding this comment.
I still think functions should be open if the command is visible and that the state should reflect that, but if the refactor will address that, then I'm fine with this for now
There was a problem hiding this comment.
Yup, that PR will address how top-level commands are represented in SectionState and SectionData by removing any function placeholders.
| expectError( | ||
| () => screen.getByLabelText("Checkmark With Circle Icon"), | ||
| "Unable to find a label with the text of: Checkmark With Circle Icon", | ||
| ); |
There was a problem hiding this comment.
is there a reason we're using expectError rather than expecting it to not be in the document (like we do elsewhere)?
There was a problem hiding this comment.
The screen query throws an error in this assertion and will automatically fail:
expect(screen.getByLabelText("Checkmark with Circle Icon")).not.toBeInTheDocument();
I tried this also and and the same error blocks the assertion from completing:
expect(screen.getByLabelText("Checkmark with Circle Icon")).toThrow()
That's how I decided to check the error itself with a try/catch.
There was a problem hiding this comment.
it's probably because you want to use queryByLabelText. I think we can keep the expectError function, but we should update this to match the rest of the tests
| if (processedLogLine.isTopLevelCommand) { | ||
| status = includesLineNumber(processedLogLine, failingLine) | ||
| ? SectionStatus.Fail | ||
| : SectionStatus.Pass; | ||
| } |
There was a problem hiding this comment.
I know I said this before, but do we want to reconsider moving the status calculation up higher in the code? This component would end up looking really clean because it would just use processedLogLine.status
There was a problem hiding this comment.
We can do the calculation earlier on achieve the refactor you're mentioning but I don't think it will necessarily be cleaner at this point in time. Currently, the status calculation only happens in this file and is only important for render purposes.
To get the value to propagate through, useSections would have to accept a separate failingLineNumber prop, and FunctionEntry, CommandEntry, SubsectionHeader and SectionHeader would all need to push along the value.
There was a problem hiding this comment.
Could we file a non-release blocking ticket to do the refactor? I think it makes sense for a section to inherently know its own status and could be useful to have in the future.
There was a problem hiding this comment.
@minnakt I think we should make this ticket when we need the SectionStatus to do operations on ProcessedLogLines. I don't think the refactor will have a great benefit until then since includesLineNumber is a constant time operation.
There was a problem hiding this comment.
it's less about the performance benefits (I assume there are none) and more that I think it makes sense to have all of this information in one place. I can make a ticket since I feel pretty strongly about it
| if (!renderCommands) { | ||
| // The function is closed. Skip all log lines until the end of the function. |
There was a problem hiding this comment.
I'm not sure if this comment is clear after the updates (i.e. why does !renderCommands mean the function is closed?)
There was a problem hiding this comment.
Good point, I updated the comment.
| const isFuncOpen = | ||
| (sectionState[func?.functionID]?.isOpen || | ||
| func?.containsTopLevelCommand) ?? | ||
| false; |
There was a problem hiding this comment.
I still think functions should be open if the command is visible and that the state should reflect that, but if the refactor will address that, then I'm fine with this for now
| if (processedLogLine.isTopLevelCommand) { | ||
| status = includesLineNumber(processedLogLine, failingLine) | ||
| ? SectionStatus.Fail | ||
| : SectionStatus.Pass; | ||
| } |
There was a problem hiding this comment.
it's less about the performance benefits (I assume there are none) and more that I think it makes sense to have all of this information in one place. I can make a ticket since I feel pretty strongly about it
DEVPROD-9626
Description
These code changes allow rendering a command outside of a function. During section parsing, function entries will be generated for top-level commands and they will be tagged with
containsTopLevelCommand. This way, log searching and section toggling can make calculations based on the property that all functions have at least 1 command and during render time, function headers that contain a top-level command will be skipped.Reach out to me for a log URL to test in beta.