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

[SR] Component integration tests #36871

Merged

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented May 22, 2019

This PR adds the component integration tests for Snapshot Repositories app.

While doing the tests I also updated the client side validation of the repository name and updated the Chrome extension to take into account all the different access paths to a DOM node.

E.g. Before, it only returned either the complete path or the leaf.
'grandParent.parent.child' or 'child'
Now it returns all the combination to access a node
'grandParent.parent.child', 'grandParent.child', 'grandParent.parent' 'parent.child' & 'child'

There is also now a "depth" config in the extension to limit the nested paths returned. 95% of the time a "parent -> child" access is enough to target a dom node.

Run the tests locally

In order to run the 3 component integration jest files, we need the latest react version 16.9.0 which is currently in alpha release. This next version brings asynchronous to the act() method which is needed to test our "hooked" components...

Dependencies

For now, in order to run the tests, make the following modifications in the package.json

root package.json

  • "react": "^16.8.0" ---> "react": "16.9.0-alpha.0"
  • "react-dom": "^16.8.0" ---> "react-dom": "16.9.0-alpha.0"

x-pack package.json

  • "react": "^16.8.0" ---> "react": "16.9.0-alpha.0"
  • "react-dom": "^16.8.0" ---> "react-dom": "16.9.0-alpha.0"

And then run yarn install in each folder.

This implies that the CI will never turn green until we don't upgrade to 16.9.0.

Another note about dependencies: when running the tests we get the warning Warning: componentWillMount is deprecated and will be removed in the next major version (...) Please update the following components: MemoryRouter, Route, Router, Switch. Updating to react-router v5.0.0 (which is backward compatible) removes the warning.... but break 3 of our apps (remote clusters, ccr and rollups) as they depend on the non-official old "Context" to access the router. The fix should not be too complex but maybe other plugins have the same issue and we probably want to make sure all our plugins are compatible with react-router v5 and allow ourselves updating to react 17.0.0 when needed (I know, this won't happen tomorrow 😊).

@jen-huang @silne30 ready to review!

CC @elastic/kibana-platform for the dependencies note .
@elastic/kibana-qa team for a preview on what we'll discuss
@alisonelizabeth as you might use this as a starting point to test watcher.

@sebelga sebelga added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more non-issue Indicates to automation that a pull request should not appear in the release notes Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI test-jest-integration labels May 22, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jen-huang jen-huang self-requested a review May 22, 2019 16:45
@sebelga sebelga changed the title [SR] Component integration tests WIP [SR] Component integration tests May 23, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

plugins: AppPlugins
): Promise<void> => {
render(<ReactApp core={core} plugins={plugins} />, elem);
export const renderReact = async (elem: Element, core: AppCore, plugins: AppPlugins) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of providing the dependencies through props, I made it through a Provider component. This made is easier to test as I could wrap the component under test with this Providers Component.

export const useAppDependencies = () => useContext<AppDependencies>(DependenciesContext);
export const setAppDependencies = (deps: AppDependencies) => {
DependenciesContext = createContext<AppDependencies>(deps);
return DependenciesContext.Provider;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create the context and return the Provider for it.

@@ -30,7 +30,7 @@ export const sendRequest = async ({
try {
const response = await httpService.httpClient[method](path, body);

if (!response.data) {
if (typeof response.data === 'undefined') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to change this as a 200 response with empty string '' was taken as an error.

@@ -44,7 +44,7 @@ export const sendRequest = async ({
};
} catch (e) {
return {
error: e,
error: e.response ? e.response : e,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to change this as the mocked error is under a "response" property.

};

const clickSnapshotAt = async (index: number) => {
const { rows } = testBed.table.getMetaData(SNAPSHOT_TABLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: destructure testBed
const { table, router, component } = testBed

expect(exists('reloadButton')).toBe(true);

await act(async () => {
find('reloadButton').simulate('click');
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about adding this to the helpers file?


expect(server.requests.length).toBe(totalRequests + 1);
expect(server.requests[server.requests.length - 1].url).toBe(
'/api/snapshot_restore/repositories'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could import the API_BASE_PATH const and use it here
${API_BASE_PATH}repositories

const repositoryName = rows[0].columns[1].value;

expect(latestRequest.method).toBe('DELETE');
expect(latestRequest.url).toBe(`/api/snapshot_restore/repositories/${repositoryName}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

expect(find('repositoryDetail.title').text()).toEqual(repo1.name);
});

test('should show a loading while fetching the repository', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing word here? maybe should show a loading state...

form.selectCheckBox('compressToggle');

const error = {
statusCode: 400,
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like setSaveRepositoryResponse is actually checking for status not statusCode

export const nextTick = (time = 0) => new Promise(resolve => setTimeout(resolve, time));

export const getRandomNumber = (range: { min: number; max: number } = { min: 1, max: 20 }) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should the getRandomString util be added to this file as well, rather than in strings.ts?

const navigateTo = (_url: string) => {
const url =
_url[0] === '#'
? _url.replace('#', '') // remove the begging hash as the memory router does not understand them
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: type begging ---> beginning

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sebelga
Copy link
Contributor Author

sebelga commented Jun 13, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sebelga
Copy link
Contributor Author

sebelga commented Jun 13, 2019

@jen-huang @alisonelizabeth I have made the CR changes suggested. Could you have another look at the PR? Cheers! 😊

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sebelga sebelga merged commit 941cc44 into elastic:master Jun 13, 2019
@sebelga sebelga deleted the test/repositories_component_integration branch June 13, 2019 18:34
sebelga added a commit to sebelga/kibana that referenced this pull request Jun 13, 2019
@cjcenizal cjcenizal mentioned this pull request Sep 30, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI non-issue Indicates to automation that a pull request should not appear in the release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more test-jest-integration v7.2.0 v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants