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

Dojo Test Renderer #710

Merged
merged 15 commits into from Apr 15, 2020
Merged

Dojo Test Renderer #710

merged 15 commits into from Apr 15, 2020

Conversation

agubler
Copy link
Member

@agubler agubler commented Mar 31, 2020

Type: feature

The following has been addressed in the PR:

Description:

Introduces a new testing module, @dojo/framework/testing/renderer, intended to retire the existing @dojo/framework/testing/harness, the harness has been deprecated but not removed.

The existing harness is has been moved to the framework/testing/harness folder.

Features:

  • Refined the existing harness API to promote testing best practices (removed expectPartial and getRender).
  • Introduces "wrapped test node" in order to interact with the assertion template and test renderer APIs instead of string selectors.
  • Built in Type-safe support for triggering widget properties.
  • Built in type-safe support for testing functional children.
  • Makes assertion templates a first class citizen, the only way to test widgets.
  • Type-safe assertion template API.
  • Inline custom comparators using the compare function in combination with a wrapped testing node
  • Output diffs in tsx format over v() and w()
import {,tsx } from '@dojo/framework/core/vdom';
import renderer, { wrap, compare } from '@dojo/framework/testing/renderer';
import assertionTemplate from '@dojo/framework/testing/assertionTemplate';

import MyWidget from './MyWidget';
import Button from './Button';
import Card from './Card';

// create a wrapped node for anything that needs
// to be used with the renderer or assertion template api
const WrappedButton = wrap(Button);
const WrappedRoot = wrap('div');
const WrappedCard = wrap(Card);

// create the base template with the wrapped nodes
// in place of the real node/widget
const baseTemplate = assertionTemplate(() => (
    // use `compare` with any property on a wrapped node
    // to pass a custom comparator
    <WrappedRoot id={compare((actual) => typeof actual === 'string'}>
        <WrappedButton onClick={() => {}} />Show</WrappedButton>
    </WrappedRoot>
));

// initialise the test renderer
const r = renderer(() => <MyWidget />);

// always expect the default render state
r.expect(baseTemplate);

// register calling a property using a wrapped node
r.property(WrappedButton, 'onClick');
// describe how to resolve functional children
r.child(WrappedCard, [{ header: [], content: [] }]);
// create a new template using the wrapped nodes
// to target the changes required
const cardTemplate = baseTemplate
    .setProperty(WrappedRoot, 'classes', [css.open])
    .insertAfter(WrappedButton, () => (
        <Card>{{
            header: () => <h1>Header</h1>,
            content: () => <div>Content</div>
        }}
        </Card>
    ));
// expect the against the updated template
// the properties will be called before a the
// widget re-renders
r.expect(cardTemplate);

r.property(WrappedButton, 'onClick');
r.expect(baseTemplate);

Note: Testing render props is not possible with the new test renderer and recommended to migrate these widgets to use functional children for full test renderer support.

Resolves #535
Resolves #529

@agubler agubler changed the title [WIP ]Dojo Test Renderer [WIP] Dojo Test Renderer Mar 31, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 31, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ae31cbd:

Sandbox Source
brave-jackson-ztozu Configuration

h.trigger('@button', 'onClick');
const r = renderer(() => <Action fetchItems={fetchItems} />);
r.expect(template);
r.property(WrappedButton, 'onClick');
Copy link
Member

@devpaul devpaul Apr 2, 2020

Choose a reason for hiding this comment

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

It might be worth mentioning in the documentation below how the onClick property is found in the renderer using the wrapped button. It's not immediately clear how the property method is using the WrappedButton to find the real Button on the rendered widget when the rendered widget isn't constructed using the wrapped widgets.

Copy link
Member Author

@agubler agubler Apr 2, 2020

Choose a reason for hiding this comment

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

That seems reasonable, I'll add something into the supplemental docs.

Copy link
Member Author

@agubler agubler Apr 3, 2020

Choose a reason for hiding this comment

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

@devpaul I have added:

The test renderer uses the location of a wrapped test node in the expected tree to attempt to perform the requested action (either r.property() or r.child()) on the actual output of the widget under test. If the wrapped test node does not match the corresponding node in the actual output tree then no action will be performed and the assertion will report a failure.

To the Wrapped Test Nodes section.

Copy link
Member

@devpaul devpaul Apr 8, 2020

Choose a reason for hiding this comment

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

Thanks for adding this. It'll be a good reference!

@devpaul
Copy link
Member

devpaul commented Apr 2, 2020

What happens if a wrapped widget is used more than once?

const WrappedButton = wrap(Button);
const template = assertionTemplate(() => (
	<div classes={[css.root]}>
		<WrappedButton key="button1" onClick={() => {}}>
			Fetch
		</WrappedButton>
		<WrappedButton key="button2" onClick={() => {}}>
			Fetch
		</WrappedButton>
	</div>
));
const r = renderer(() => <MyWidget buttonActions={[ jest.fn(), jest.fn() ]} />);
r.expect(template);
r.property(WrappedButton, 'onClick');

Let's say that MyWidget has two buttons exactly like the assertion template and buttonActions are assigned respectively to each Button's onclick. Are both mock functions called? Is an error produced? What happens?

dylans
dylans previously requested changes Apr 2, 2020
Copy link
Member

@dylans dylans left a comment

very minor suggestions (reviewed the docs more than the code)... I assume someone else is reviewing the code here.

docs/en/testing/introduction.md Outdated Show resolved Hide resolved
docs/en/testing/introduction.md Outdated Show resolved Hide resolved
docs/en/testing/introduction.md Outdated Show resolved Hide resolved
@agubler
Copy link
Member Author

agubler commented Apr 2, 2020

What happens if a wrapped widget is used more than once?

const WrappedButton = wrap(Button);
const template = assertionTemplate(() => (
	<div classes={[css.root]}>
		<WrappedButton key="button1" onClick={() => {}}>
			Fetch
		</WrappedButton>
		<WrappedButton key="button2" onClick={() => {}}>
			Fetch
		</WrappedButton>
	</div>
));
const r = renderer(() => <MyWidget buttonActions={[ jest.fn(), jest.fn() ]} />);
r.expect(template);
r.property(WrappedButton, 'onClick');

Let's say that MyWidget has two buttons exactly like the assertion template and buttonActions are assigned respectively to each Button's onclick. Are both mock functions called? Is an error produced? What happens?

@devpaul at the moment it would actually call the corresponding onClick of both the button widgets in the widget under tests rendered output. This actually feels incorrect to me and it should only call one, basically you shouldn't use the same wrapped node twice in the assertion structure. If we change to follow that rule then it would only call the first wrapped node it finds and no more. This would then lead to an assertion failure in the test as it wouldn't have called the second onClick

@agubler agubler changed the title [WIP] Dojo Test Renderer Dojo Test Renderer Apr 2, 2020
@agubler agubler force-pushed the harness-rewrite branch 2 times, most recently from 15b0d1d to c344242 Compare Apr 3, 2020
@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #710 into master will decrease coverage by 0.05%.
The diff coverage is 97.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #710      +/-   ##
==========================================
- Coverage   97.78%   97.72%   -0.06%     
==========================================
  Files         121      125       +4     
  Lines        7050     7485     +435     
  Branches     1603     1718     +115     
==========================================
+ Hits         6894     7315     +421     
- Misses        156      170      +14     
Impacted Files Coverage Δ
src/testing/assertRender.ts 95.65% <95.65%> (ø)
src/testing/renderer.ts 97.51% <97.51%> (ø)
src/testing/decorate.ts 98.00% <98.00%> (ø)
src/core/vdom.ts 97.98% <100.00%> (+<0.01%) ⬆️
src/testing/harness/assertionTemplate.ts 99.15% <100.00%> (ø)
src/testing/harness/harness.ts 91.36% <100.00%> (ø)
src/testing/harness/support/assertRender.ts 98.23% <100.00%> (ø)
src/testing/harness/support/selector.ts 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cead544...ae31cbd. Read the comment docs.

@devpaul
Copy link
Member

devpaul commented Apr 4, 2020

This actually feels incorrect to me and it should only call one, basically you shouldn't use the same wrapped node twice in the assertion structure

Sounds like it would be reasonable to throw an exception and fail the test at this point. It would give us a chance to provide a good error message that educates the user on proper usage. If we proceed there's a chance that it might work for their test (i.e. maybe they only check the first value).

@agubler
Copy link
Member Author

agubler commented Apr 8, 2020

Sounds like it would be reasonable to throw an exception and fail the test at this point. It would give us a chance to provide a good error message that educates the user on proper usage. If we proceed there's a chance that it might work for their test (i.e. maybe they only check the first value).

@devpaul the test renderer will now throw an error if the same wrapped test node is detected multiple times during an assertion

maier49
maier49 approved these changes Apr 9, 2020
- `renderFunction`: A function that returns a `WNode` for the widget under test
- [`customComparators`](/learn/testing/dojo-test-harness#custom-comparators): Array of custom comparator descriptors. Each provides a comparator function to be used during the comparison for `properties` located using a `selector` and `property` name
- `options`: Expanded options for the harness which includes `customComparators` and an array of middleware/mocks tuples.
The test renderer uses the location of a wrapped test node in the expected tree to attempt to perform the requested action (either `r.property()` or `r.child()`) on the actual output of the widget under test. If the wrapped test node does not match the corresponding node in the actual output tree then no action will be performed and the assertion will report a failure.
Copy link
Contributor

@maier49 maier49 Apr 9, 2020

Choose a reason for hiding this comment

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

It seems like this statement is made out of context here

Copy link
Member Author

@agubler agubler Apr 9, 2020

Choose a reason for hiding this comment

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

@maier49 It's trying to explain how the wrapped nodes are used, is there a more appropriate place outside of the test node section?

Copy link
Member Author

@agubler agubler Apr 9, 2020

Choose a reason for hiding this comment

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

Updated


## Harness API
Working with [assertion templates](/missing/link) and the test renderer is done using [wrapped test nodes](/missing/link) that are defined in the assertion templates structure, ensuring type safety throughout the testing life-cycle.
Copy link
Contributor

@maier49 maier49 Apr 9, 2020

Choose a reason for hiding this comment

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

Are these links coming in later PRs?

docs/en/testing/supplemental.md Outdated Show resolved Hide resolved
docs/en/testing/supplemental.md Outdated Show resolved Hide resolved
docs/en/testing/supplemental.md Outdated Show resolved Hide resolved
docs/en/testing/supplemental.md Outdated Show resolved Hide resolved
docs/en/testing/supplemental.md Outdated Show resolved Hide resolved
docs/en/testing/supplemental.md Outdated Show resolved Hide resolved
docs/en/testing/supplemental.md Outdated Show resolved Hide resolved

Given the following VDOM structure:
In addition to asserting the output from a widget, widget behavior can be tested by using the `renderer.property()` function. The `property()` function takes a [wrapped test node]() and the key of a property to call before the next call to `expect()`.
Copy link
Contributor

@maier49 maier49 Apr 9, 2020

Choose a reason for hiding this comment

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

It can take additional arguments to pass to the function still right? Maybe worth mentioning

Copy link
Member Author

@agubler agubler Apr 9, 2020

Choose a reason for hiding this comment

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

Updated

tomdye
tomdye approved these changes Apr 15, 2020
@agubler agubler dismissed dylans’s stale review Apr 15, 2020

addressed feedback

@agubler agubler merged commit edbf841 into dojo:master Apr 15, 2020
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants