Initiatives: create Initiative from discourse topics #1480
Conversation
| jest.mock("./htmlTemplate"); | ||
| const spyParseCookedHtml = ((parseCookedHtml: any): JestMockFn<any, any>); | ||
|
|
||
| function givenParseError(message: string) { |
There was a problem hiding this comment.
You could simplify the testing if we split initiativeFromDiscourseTracker into two functions, a wrapper and an internal. the wrapper does the most basic validation (check serverUrl), and calls parseCookedHTML on the post's cookedHtml, and passes the results into the internal method.
then, to test the internals, we don't need to mock parseCookedHTML, we can test the internal method directly on the expected inputs. and the wrapper, we only need to check that it calls parseCookedHTML faithfully (which IMO flow gets for us, i don't think we even need to unit test it), and we test the validation logic on the wrapper.
what do you think of this change? i don't think it's necessary, but it would be cleaner.
another approach could be: write a method that, given the intended initiative partial template, produces the raw HTML to put in the post. then the test would be more "end-to-end". i don't think this is the best approach because of the complexity it adds to the test code, but it's interesting to think about.
There was a problem hiding this comment.
Sure, though instead of splitting the implementation, I would then pass parseCookedHtml as a function argument, curried or as a constructor argument for a class. We can create a flow type for the function signature of parseCookedHtml. I was wondering where the limit was for getting utility from interfaces vs just mocking the one usage. 😄
There was a problem hiding this comment.
Yeah. For the record, I"m also OK with the current implementation, but I think making it part of the interface somehow would be cleaner. :)
There was a problem hiding this comment.
I've updated this to take parseCookedHtml as an argument instead of mocking the module.
Beanow
force-pushed
the
wip/initiativeFromDiscourseTracker
branch
from
76e913e to
296b6b0
Compare
| if (url.startsWith("/")) { | ||
| return `${serverUrl}${url}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
teensy nit: i would remove the linebreak so the whole function block scans as a single paragraph. change not needed.
Beanow
force-pushed
the
wip/initiativeFromDiscourseTracker
branch
from
296b6b0 to
cc28ad1
Compare
Beanow
force-pushed
the
wip/htmlTemplate
branch
from
5224e05 to
209db1c
Compare
Beanow
force-pushed
the
wip/initiativeFromDiscourseTracker
branch
from
cc28ad1 to
b0db049
Compare
Depends on #1478.
This plus the #1478 PR replace the previous #1418.
This adds the Discourse specific semantics on top of the basic template parsing.
Test plan:
yarn unitNo usage available for manual tests.