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

Adds integration tests to client website #863

Conversation

atomicgamedeveloper
Copy link
Contributor

This PR relates to issue #826 about testing the Library page of the client.

@atomicgamedeveloper
Copy link
Contributor Author

@prasadtalasila Is this the intended interaction between the assetType and scope tabs? I.e. the first scope tab gets selected when you select an assetType tab.

Recording.2024-07-08.190052.mp4

@prasadtalasila
Copy link
Contributor

@atomicgamedeveloper yes, that is correct.

@atomicgamedeveloper
Copy link
Contributor Author

Ok, thank you. I will capture this in a test. Also I will see to removing the no-plusplus Error and cut down the file length to below 250.

@prasadtalasila
Copy link
Contributor

Ok, thank you. I will capture this in a test. Also I will see to removing the no-plusplus Error and cut down the file length to below 250.

Thanks

client/test/integration/DigitalTwins.test.tsx Outdated Show resolved Hide resolved
<Workbench />
</MemoryRouter>
);
beforeEach(() => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

collapseWhitespace: false,
});

beforeEach(() => {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

Copy link
Contributor

@prasadtalasila prasadtalasila left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Please resolve the codeclimate issues as well.

});

it('changes iframe src according to the the selected tab', () => {
for (let i = 0; i < tabs.length; i += 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please name the variable more appropriately

import * as React from 'react';
import {
getDefaultNormalizer,
fireEvent,
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned about user-event. Is it possible to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not easily due to awaits in loops being disallowed by the ESLint rules, but I will give it another try.


it('selects the first scope tab when you select an assetType tab', () => {
// i is 1 as the first tab is already selected so clicking on it will not have an effect on the subtabs
for (let i = 1; i < assetType.length; i += 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a better variable name here

selected: isFirstAssetTypeTab,
});
fireEvent.click(assetTypeTab);
for (let j = 0; j < scope.length; j += 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use a better variable name

@prasadtalasila
Copy link
Contributor

prasadtalasila commented Jul 18, 2024

@atomicgamedeveloper glad to see the improvements in test coverage.
There are some warnings/errors in the tests. Please see the github actions log of testing react website.

Copy link
Contributor

@prasadtalasila prasadtalasila left a comment

Choose a reason for hiding this comment

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

@atomicgamedeveloper thanks for the PR updates. The tests on the following files can be improved.

----------------------------------|---------|----------|---------|---------|----------------------
File                              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s    
----------------------------------|---------|----------|---------|---------|----------------------
Title.tsx                       |       0 |      100 |       0 |       0 | 1-16                 
AccountTabData.tsx              |     100 |    79.41 |     100 |     100 | 48-52,73             
AuthProvider.tsx                |       0 |        0 |       0 |       0 | 1-19                 
PrivateRoute.tsx                |   83.33 |    73.33 |     100 |   83.33 | 22,28,30,41          
Signin.tsx                      |   63.63 |      100 |       0 |   63.63 | 10-16                
WaitAndNavigate.tsx             |      30 |        0 |       0 |   31.57 | 9-29                 
src/util                         |   82.05 |    77.77 |   53.33 |   81.57 |                      
src/util/auth                    |   26.08 |       30 |    12.5 |   26.66 |                      
Authentication.ts               |    37.5 |       60 |   16.66 |    37.5 | 26-49,54-58          
useOidcConfig.ts                |       0 |        0 |       0 |       0 | 1-44

Please check the comments as well.

client/src/page/DrawerComponent.tsx Show resolved Hide resolved
client/jest.integration.config.json Outdated Show resolved Hide resolved
client/src/routes.tsx Outdated Show resolved Hide resolved
client/test/integration/jest.setup.ts Outdated Show resolved Hide resolved
client/jest.config.json Outdated Show resolved Hide resolved
client/test/unitTests/Page/Menu.test.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

since unitTests/__mocks__ are used in integration tests too, is there a benefit in moving them to ``test/` directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is nice. Perhaps mocks placed in test/__mocks__ be imported like this`?

@prasadtalasila
Copy link
Contributor

@atomicgamedeveloper

The client/src/page/Title.tsx has been removed. Is it not used anywhere else?

@atomicgamedeveloper
Copy link
Contributor Author

@atomicgamedeveloper

The client/src/page/Title.tsx has been removed. Is it not used anywhere else?

It is not referenced anywhere in the project as far as I can tell from searching the codebase.

@prasadtalasila
Copy link
Contributor

@atomicgamedeveloper The PR looks ready to merge. Please make the following changes.

  1. Please increment the package version to 0.3.6 in client/package.json
  2. The tests are taking too long. If there is a clean solution to reduce test time, please do so. Otherwise, please increment jest test time out to avoid false timeout errors.

Copy link

codeclimate bot commented Aug 7, 2024

Code Climate has analyzed commit 0c78ccb and detected 0 issues on this pull request.

View more on Code Climate.

@prasadtalasila prasadtalasila merged commit a335ae4 into INTO-CPS-Association:feature/distributed-demo Aug 7, 2024
3 checks passed
@prasadtalasila prasadtalasila changed the title Library page of Client Integration Test Adds integration tests to client website Aug 7, 2024
@atomicgamedeveloper atomicgamedeveloper deleted the 826-integration-test-client branch August 8, 2024 21:03
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.

2 participants