Handles https://linear.app/comm/issue/ENG-2000/add-unit-tests-to-test-the-regex-for-spoiler-text

Thanks for creating this, looks like a great first diff to me! Some suggestions (feel free to defer to @atul before implementing any of these since he has much more experience in this):

1. Can we get the it(...) messages to be a little more descriptive for each individual test? For a reader opening the file, it would make it easier to know what exactly is being tested and what is expected. I think a good example of this is in thread-utils.test.js. So for example in your last one, we could maybe do something like it('should not match anything when there is a new line between the spoiler characters). Also, if a test fails it would make it easier to understand why probably.

2. Not super familiar with testing RegEx, but is it good practice / possible to actually test the match so we can see if we are matching what is expected?

3. IF we can / should test what the actual match is, it would be good to add a match when there is multiple spoilers within one single sentence.

4. Minor observation, but you've left out some characters when writing out the actual Regex, the actual RegEx is /^\|\|([^\n]+?)\|\|/g.

Overall looks great to me, nice work. I'll take a second pass over this a little later and update with any comments if I have any more.

Hi @rohan, thank you for your review. To follow up on your points:
1: I've changed the it statements to be more descriptive.
2/3: I'm a bit confused on what you mean by "to actually test the match so we can see if we are matching what is expected" - could you please clarify?
4: I was getting a eslint/prettier problem when I tried to include the actual RegEx in the it statement, so I moved it to a comment. Is this okay?

Also, I committed my changes and ran arc diff, but for some reason, the new changes can be seen here: https://phab.comm.dev/D5447, not in this thread. Is this the correct way to revise a diff? Or is there a way to include it in the current running thread until it lands?

Also, I committed my changes and ran arc diff, but for some reason, the new changes can be seen here: https://phab.comm.dev/D5447, not in this thread. Is this the correct way to revise a diff? Or is there a way to include it in the current running thread until it lands?

Ah it looks like you made a new commit so when you ran arc diff it created a new "diff."

What you want to do instead is

1. Address feedback
2. Update commit in place with git commit --amend
3. Run arc diff to update diff

We can go over this during call later today

In D5358#160612, @manan wrote:

Hi @rohan, thank you for your review. To follow up on your points:
1: I've changed the it statements to be more descriptive.

Thanks for following up!

2/3: I'm a bit confused on what you mean by "to actually test the match so we can see if we are matching what is expected" - could you please clarify?

So I was basically wondering if we can test whether the RegEx matches what we are actually looking for. So for example, if we are doing something like expect(||hello||).toMatch(spoilerRegex), is there a way to test whether the RegEx is extracting out the "spoiler text", in this case hello?

4: I was getting a eslint/prettier problem when I tried to include the actual RegEx in the it statement, so I moved it to a comment. Is this okay?

Also, I committed my changes and ran arc diff, but for some reason, the new changes can be seen here: https://phab.comm.dev/D5447, not in this thread. Is this the correct way to revise a diff? Or is there a way to include it in the current running thread until it lands?

Sure it was just a minor thing I noticed! In terms of the arc diff, if you run arc diff on the branch where your original commit was, I think it pulls the top-level commit by default. So if you made a new commit instead of amending this one, that's probably why it made a new one. That's just my thoughts though, @atul might be able to confirm/explain

Edit: Seems like he responded above!

So I was basically wondering if we can test whether the RegEx matches what we are actually looking for. So for example, if we are doing something like expect(||hello||).toMatch(spoilerRegex), is there a way to test whether the RegEx is extracting out the "spoiler text", in this case hello?

Discussed with Atul, going to land this diff as is and will address your comment in a follow up diff.