Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Add some more tests for APIRef #1122

Merged
merged 7 commits into from
Apr 18, 2019
Merged

Conversation

wbamberg
Copy link

This PR contains tests for the APIRef macro. I'm writing tests as part of mdn/sprints#1313: after this is merged the next step would be to update the macro (and its tests) to handle the way we are now representing events.

I don't think this tests every edge case. But APIRef's behavior is quite complex especially as it's very dependent on various data sources (subpages, GroupData, InterfaceData, L10n-Common). So I've tried to cover the main cases while trying to keep the size of test under control.

I've been guided by this comment: #1080 (comment) in writing this test, so I thought it might be worth going through that here:


say the current page is at /docs/Web/API/MainIF/, is mainIF correctly given as "MainIF"?

Tested: we set this as the slug and check the result.

say the current page is at /docs/Web/API/MainIF/something-else, is mainIF correctly given as "MainIF"?

Tested: we try setting the slug to a subpage and check the result.

given a value of mainIF and a particular subtree under that page, are "ctors/"properties"/"methods" being handled correctly? e.g.

  • if there are no ctors, is the section empty? If there is one, is there one item? if there are two, are they both there?
  • if the subpages have tags for "experimental" &c, are the badges being shown.

Tested: we have test data with no ctors, one property, three methods. We have tests for no badges, on badge, two badges, and all the badges.

there is some tricky string-fiddling in here (e.g. https://github.com/mdn/kumascript/blob/master/macros/APIRef.ejs#L140-L143) which always makes me nervous. Are there strings we might encounter that would break the HTML output?

We have tests for the bit of code referred to here, as some of our example pages use TestInterface.MethodName in the title.

is the CTA correctly added for non-en-US locales?

Tested: the fr locale doesn't have translations, and we check that the link text includes the CTA ("[Traduire]").

given a parameter value to use for GroupData, check that "related" and "events" are calculated properly:

Tested: we have a sample GroupData that includes interfaces, methods, and events.

what if GroupData doesn't have an entry for the given parameter?

Tested: we test passing a different argument, that doesn't match a GroupData entry.

what if it has various different possible combinations of interfaces, methods, properties, events?

Sort of tested: we only have one GroupData test file, but it contains a mix of stuff.

what if the names of these things contain weird characters? Will we break the HTML? (actually I know the answer to this: yes we will, but the GroupData test should ensure that doesn't happen. But perhaps APIRef should also be defensive.)

This isn't explicitly tested. I know that the macro will give bad output if GroupData contains bad strings. But we separately test the contents of GroupData, so it should be OK. We might want to consider fixing this, but I decided not to add it in this particular effort.

given some different values for InterfaceData, do we calculate and show inherited and implemented correctly? Does the logic in getInheritance and getImplementedBy work properly, for different values?

We test with a sample InterfaceData file that contains some inherited and implemented-by items.

In general I think we should pay attention to anywhere where there's string-fiddling going on, and/or where we're getting string data from somewhere and sticking it in HTML output.

See above. This is somewhat tested but not explicitly, just to keep this test from getting even bigger.


I've also fixed a bug in the macro that these tests found.

@wbamberg wbamberg requested a review from escattone April 18, 2019 04:48
@wbamberg wbamberg changed the title More apiref tests Add some more tests for APIRef Apr 18, 2019
Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is beautifully done. I really like the way you've organized a complex set of tests, test data, and expected results, by calling each testMacro with its own config. This is definitely one of the most difficult macros to test, if not the most difficult, and you've taken a huge step forward from where we were. Saying "thanks" seems like too little. Really nice work @wbamberg, we owe you a debt of gratitude!

tests/macros/apiref.test.js Show resolved Hide resolved
@escattone escattone merged commit 850cf45 into mdn:master Apr 18, 2019
@wbamberg wbamberg deleted the more-apiref-tests branch April 18, 2019 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants