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

Fix: Conditionally strip-out markdown in note previews #1839

Merged
merged 2 commits into from
Jan 17, 2020

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Jan 16, 2020

Tests have been failing because noteTitleAndPreview is reported to be taking
longer than 1ms to run. In fact, Date.now(), which we were using to validate
this, includes reduced time precision for protection against timing attacks. In
this patch I've done two things to address the failing tests:

  • Replace Date.now() with process.hrtime() which gives us nanosecond
    precision
  • Run the function-under-test 100 times in order to eliminate variance in the
    measurement

While fixing this test I noted a flaw in noteTitleAndPreview that we missed
in #1647. We created the helper function removeMarkdownFromOutput which
transforms a string based on whether or not markdown is enabled for the note.
Unfortunately for us we were setting a module-global function based on the
module-init-time value isMarkdown, which as a function is always truthy,
meaning that it would return a function that alwasy removed markdown regardless
of the note.

In this patch I've addressed this by passing stripMarkdown into that
function. In the PR for #1647 we discussed this parameter and removed it but we
kept removeMarkdownFromOutput outside the scope of noteTitleAndPreview. We
could leave out the parameter and move the function into noteTitleAndPreview
or we could add the parameter back.

For the purpose of only creating that function once I have added the parameter
back in. noteTitleAndPreview gets called in some inner loops and I preferred
to avoid recreating the filtering function on every call, or at least until the
JIT figured it out; I'm sure that the performance impact is insignificant but I
don't see any benefit to one way over the other and I'd rather do less work as
the default.

Sorry @belcherj for not catching this in the original PR

Testing

The test-fix will run itself in CircleCI. You can validate the note list behavior for
notes with and without markdown formatting enabled.

In the following screenshots the notes have the following content:

# Not Markdown

and

# Markdown

Before

Screen Shot 2020-01-15 at 11 02 48 PM

After

Screen Shot 2020-01-15 at 11 05 36 PM

@dmsnell dmsnell added the bug Something isn't working label Jan 16, 2020
@dmsnell dmsnell requested a review from a team January 16, 2020 06:06
@dmsnell dmsnell force-pushed the fix/misreported-tests branch 2 times, most recently from b2e9e3c to 846be5b Compare January 16, 2020 20:25
@codebykat
Copy link
Member

Tested this. I noticed that if you change a note from Markdown to non-Markdown, you have to reload before the difference is reflected in the notes list. Is that intentional?

@dmsnell
Copy link
Member Author

dmsnell commented Jan 16, 2020

Is that intentional?

No but probably related to our odd data flows. Unlikely related to this PR in my guess - I don't know how to confirm that since this behavior was always stripping away markdown before.

Tests have been failing because `noteTitleAndPreview` is reported to be taking
longer than 1ms to run. In fact, `Date.now()`, which we were using to validate
this, includes reduced time precision for protection against timing attacks. In
this patch I've done two things to address the failing tests:
 - Replace `Date.now()` with `process.hrtime()` which gives us nanosecond
   precision
 - Run the function-under-test 100 times in order to eliminate variance in the
   measurement

While fixing this test I noted a flaw in `noteTitleAndPreview` that we missed
in #1647. We created the helper function `removeMarkdownFromOutput` which
transforms a string based on whether or not markdown is enabled for the note.
Unfortunately for us we were setting a module-global function based on the
module-init-time value `isMarkdown`, which as a function is always truthy,
meaning that it would return a function that alwasy removed markdown regardless
of the note.

In this patch I've addressed this by passing `stripMarkdown` into that
function. In the PR for #1647 we discussed this parameter and removed it but we
kept `removeMarkdownFromOutput` outside the scope of `noteTitleAndPreview`. We
could leave out the parameter and move the function into `noteTitleAndPreview`
or we could add the parameter back.

For the purpose of only creating that function once I have added the parameter
back in. `noteTitleAndPreview` gets called in some inner loops and I preferred
to avoid recreating the filtering function on every call, or at least until the
JIT figured it out; I'm sure that the performance impact is insignificant but I
don't see any benefit to one way over the other and I'd rather do less work as
the default.
@dmsnell dmsnell force-pushed the fix/misreported-tests branch from 59b62aa to 9fdaf5b Compare January 16, 2020 21:53
@dmsnell dmsnell merged commit fd7cdc0 into develop Jan 17, 2020
@dmsnell dmsnell deleted the fix/misreported-tests branch January 17, 2020 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants