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

Add better testing coverage for comment linking #39460

Closed
roryabraham opened this issue Apr 2, 2024 · 14 comments
Closed

Add better testing coverage for comment linking #39460

roryabraham opened this issue Apr 2, 2024 · 14 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Task

Comments

@roryabraham
Copy link
Contributor

@perunt this issue is a placeholder to implement a bunch of UI tests to cover comment linking functionality

@perunt
Copy link
Contributor

perunt commented Apr 3, 2024

Currently, we have one end-to-end test for opening a link in another chat. I have a few ideas for these tests:

  1. Open a link from Chat A to Chat B if messages are preloaded in Onyx.
    2-3. Open a link from Chat A to Chat B if messages are preloaded in Onyx and located on the edge of preloading. This will help us know if we properly trigger fetch more and if the focus on the linked message is preserved. Also, do the same for the opposite side.
  2. Open a link from Chat A to Chat B for an image message.
  3. Open a link from Chat A to Chat B for a thread.
  4. Open a link from Chat A to Chat B for a message with a reaction

@roryabraham roryabraham assigned perunt and roryabraham and unassigned perunt Apr 3, 2024
@roryabraham roryabraham moved this to Todo in Comment Linking Apr 5, 2024
@roryabraham
Copy link
Contributor Author

@perunt any update here?

@melvin-bot melvin-bot bot added the Overdue label Apr 16, 2024
@roryabraham
Copy link
Contributor Author

@perunt is OOO, definitely hoping that we complete this before he jumps into anything else though

@melvin-bot melvin-bot bot removed the Overdue label Apr 16, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 25, 2024
@roryabraham
Copy link
Contributor Author

pretty sure @perunt will be back from vacation and able to work on this soon

@melvin-bot melvin-bot bot removed the Overdue label Apr 27, 2024
@roryabraham
Copy link
Contributor Author

@perunt I'm looking at doing a significant refactor of comment linking - this is a very important issue to work on to provide better stability for this core feature

@melvin-bot melvin-bot bot removed the Overdue label May 7, 2024
@melvin-bot melvin-bot bot added the Overdue label May 15, 2024
@roryabraham
Copy link
Contributor Author

@melvin-bot melvin-bot bot removed the Overdue label May 23, 2024
@janicduplessis
Copy link
Contributor

I have some extra unit tests for getContinuousReportActionChain to cover some edge cases around inserting random actions. I have a few more cases in mind too that I want to add.

After that it would be great to add some UI tests to have more complete test coverage of comment linking flows.

@melvin-bot melvin-bot bot added the Overdue label Jun 3, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jun 17, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

This issue has not been updated in over 15 days. @roryabraham, @perunt eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot removed the Overdue label Jun 17, 2024
@perunt
Copy link
Contributor

perunt commented Jun 18, 2024

Not overdue, I just started working on it. I'ill push some tests soon

@roryabraham
Copy link
Contributor Author

@janicduplessis has been working on UI tests in the new client-side gap detection PR

@perunt
Copy link
Contributor

perunt commented Jun 23, 2024

Is it still needed in the E2E tests?
Also, do we have a list of issues related to comment linking? I just want to check if my finding has already been captured

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Monthly KSv2 labels Jul 17, 2024
@melvin-bot melvin-bot bot changed the title Add better testing coverage for comment linking [HOLD for payment 2024-07-24] Add better testing coverage for comment linking Jul 17, 2024
Copy link

melvin-bot bot commented Jul 17, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.7-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-07-24. 🎊

For reference, here are some details about the assignees on this issue:

  • @perunt does not require payment (Contractor)

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jul 23, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

@roryabraham roryabraham changed the title [HOLD for payment 2024-07-24] Add better testing coverage for comment linking Add better testing coverage for comment linking Jul 27, 2024
@roryabraham
Copy link
Contributor Author

This is done

@melvin-bot melvin-bot bot removed the Overdue label Jul 27, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Comment Linking Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Task
Projects
Status: Done
Development

No branches or pull requests

3 participants