Reset defaults before tests

[UziTech]

Marked version: 0.3.19

Description

• add function marked.getDefaults() that gets assigned to marked.defaults
• resets the defaults before each test

Contributor

• Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
• no tests required for this PR.
• If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

• Draft GitHub release notes have been updated.
• CI is green (no forced merge required).
• Merge PR

[styfle]

This cloned object could have the same problem if it’s mutated. Instead, created a function marked.getDefaults() that returns a new object each time. That function can be used to assign the default object the first time.

[UziTech]

good call

[styfle]

Is the clone still necessary?
Can't you return defaults; here instead?
It's always returning a new object each call to the function so I can't think of a scenario where it would break.

[UziTech]

I see what you are saying

[joshbruce]

I ran into problems using setOptions, that's why we switched to making them part of the call. I was thinking we would want to do something like:

  beforeEach(function() {
    marked.defaults = {
      headerIds: false,
      xhtml: true
    }
  });

  afterEach(function() {
    marked.defaults = {
      headerIds: true,
      xhtml: false
    }
  });

The CommonMark spec doesn't recognize header IDs and uses XHTML for some of the tests.

[joshbruce]

Is it possible to do this as part of the test itself instead of the lib? Believe @styfle mentioned something like that before. Thinking it would be better to have the test use a blank object over the singleton pattern that seems preferred by Node (understandably in many use cases).

[UziTech]

@joshbruce We could do something like that in the tests but then next time we add a new test file we will have to remember to do the same thing or we will fall into the same trap. This way we never have to think about it again.

[joshbruce]

@UziTech: Agreed. Think that's on us as reviewers. Not sure how often we'll be adding new test suites at this juncture, just seems the more logical place and, if we can do it in a single place, then we should be good.

[joshbruce]

@UziTech: PS. good catch on the not returning thing. I mean to ask about that.

[styfle]

LGTM 👍

[styfle]

@joshbruce I think this PR should land before the other PRs that involve the test harness.

[joshbruce]

Just want to verify the order of operations here. Before each, we set the options to defaults, the tests modify where needed. We are also making the defaults a getter only because we have the setter via setOptions.

[UziTech]

Yes, marked.getDefaults() will always return our default options. marked.defaults is still overwritten by marked.setOptions (which I think we should change in 1.x)

We are resetting marked.defaults to marked.getDefaults() in a beforeEach that will run before any other beforeEach so we can still set options for tests in a beforeEach and it will work as expected.