Skip to content
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.

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

feat(ts/diversionPage): remove timestamp from diversion page headers #2924

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

firestack
Copy link
Member

@firestack firestack commented Jan 17, 2025

@firestack firestack marked this pull request as ready for review January 17, 2025 16:14
@firestack firestack requested a review from a team as a code owner January 17, 2025 16:14
@firestack firestack requested review from hannahpurcell and removed request for a team January 17, 2025 16:14
@firestack firestack enabled auto-merge (squash) January 17, 2025 16:14
@firestack
Copy link
Member Author

Wasn't sure what to do for tests here, anyone has any thoughts for what should be tested (as I'm not sure we should be testing the absence of something, let me know)

@hannahpurcell hannahpurcell self-assigned this Jan 17, 2025
@hannahpurcell
Copy link
Collaborator

hannahpurcell commented Jan 21, 2025

Wasn't sure what to do for tests here

Hmm! Yeah I see what you mean. It'd be peculiar to have a test to check absence of something. Maybe once it's been added to the side panel, perhaps that test could query for all instances of "On detour since" and verify it's visible, but also something else that has been updated about the element between the two versions? Is there a way to check its parent element? There is a way to check elements contained by the selected one, but that might not be enough.

Well hm, another way of thinking about it. There should have been tests to check the text variation between the 3 detour statuses, and those weren't written. If those existed, how would we have updated the test to pass here?

Copy link
Collaborator

@hannahpurcell hannahpurcell left a comment

Choose a reason for hiding this comment

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

I'm comfortable approving, but awaiting discussion on tests

@firestack
Copy link
Member Author

Well hm, another way of thinking about it. There should have been tests to check the text variation between the 3 detour statuses, and those weren't written. If those existed, how would we have updated the test to pass here?

I think we probably would've had 3 tests and would remove one with the removal of the feature? We could've also inverted the test so that we expect it to fail.

@hannahpurcell
Copy link
Collaborator

I think we probably would've had 3 tests and would remove one with the removal of the feature?

Yeah, I agree that we'd probably just remove a test. I think my philosophy, then, would be not introducing a test to verify an omission. So approving!

@firestack firestack merged commit 44f157a into main Jan 22, 2025
48 checks passed
@firestack firestack deleted the kf/asn/remove-timestamp-header branch January 22, 2025 16:50
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.

2 participants