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

Codi v1.0.x & test clean up and optimisation ⚡🐶 #1773

Merged
merged 34 commits into from
Dec 18, 2024

Conversation

RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst RobAndrewHurst commented Dec 16, 2024

Codi Test changes ⚡🐶

This pr addresses some performance issues with the current test base where the run time for tests on the v4.13.0-alpha (minor) branch takes +-40 seconds to complete. This is untenable. With this I have made changes to the codi test framework to run tests in parallel. This has come with some structural changes to the suites and tests themselves.

Note that the integrity tests need to be looked at in another PR. They will currently be not in an operational state until the milestone is complete.

Codi changes

There has been a params object introduced to the describe & it callback functions. The object is used to define the name of the suite/test, the id that can be used to identify the callback & a parentId which is used to assign a child to a parent.

The primary reason for this introduction is that all tests now execute in parallel meaning that tests will execute much faster but will execute when they can. This would create some difficulty with tracking what callback function belonged to what parent. This was done originally by referring to what function had just been executed in the TestState. This can't happen with promises, where the order of the test will always be unknown.

This change has also resulted in test suite nesting being easily done where a suite can have children and those children can also have children, making one big happy testing family.

//First Suite
codi.describe({ name: 'First Test Suite', id: 'first' }, () => { });

//Second Suite
codi.describe({ name: 'nested suite', id: 'second', parentId: 'first' }, () =>{

//Tests
codi.it({ name: 'First test', parentId: 'first' }, () => { }); //First Suite
codi.it({ name: 'Second test', parentId: 'first' }, () => { }); //First Suite
codi.it({ name: 'Third test', parentId: 'second' }, () => { });// Second Suite

});

mapview removeLayer()

I have also added in the removeLayer function that will remove layers from the mapview object by layer.key.

This will be useful for custom layers in the future, but makes it quite useful to adding different layer resources when testing different core tests.

There are some limitations which this however. If you add a layer in that has a template or query that needs to execute on the layer it will currently fail because in the mod/query.js we check for layer validation against the workspace not what is currently in the mapview. I am keen to open up a discussion to how we can overcome this, but there are a lot of potential security issues to consider.

Test Changes

All of the tests should have been re-written to use the new codi structure. A lot of the test assets have also been removed, which a handful still remaining. These remaining assets are for tests that need a new approach, but will need to be done in a new PR as this PR has gone on for long enough now :).

Structural changes to the test library have also been made where we used to create test objects with a suffix of 'test'. These suffix's have been removed in favour of just the exact mirror call of the function or object in the lib directory. (Within reason). There are some which don't really have a direct mirror in the lib directory. As well as we can't have duplicate references like mapp or ui.

Tests have also been removed or cleaned up where necessary. Removed tests need to be re-looked at when some additional blockers have been resolved.

Test Plugin

A very rudimentary plugin has been written to just get the initial core tests out of the door. Further logic and extension will be applied to this experimental plugin going forward. But for now it will just exists in the /tests/assets/plugins directory as an asset until fully finished and released into the wild.

In order to use the test plugin you can include the following on the locale of an instance

"test": {
 "core": true,
 "options": {
  "quiet": false,
  "showSummary": true 
 }
}

This will be expanded and changed in the PR's to come. This is just how you can call the core tests from a workspace for now.

A question that people can help me solve is where do we want this plugin to go? Core or the plugins repo?

@RobAndrewHurst RobAndrewHurst changed the title Test plugin Codi v1.0.x & test clean up and optimisation ⚡🐶 Dec 17, 2024
@RobAndrewHurst RobAndrewHurst marked this pull request as ready for review December 18, 2024 09:45
@RobAndrewHurst RobAndrewHurst linked an issue Dec 18, 2024 that may be closed by this pull request
Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

I'm happy with this as the first stage towards the clean up.
The tests are running faster and all seem to be working as expected.
I understand the reasons for the removal of certain tests and the testview in this PR as this is the first piece in the puzzle 🧩

@dbauszus-glx
Copy link
Member

I don't fully understand the public/_mapp.test.js library.

Should this sit in a tests subfolder in js since there is already a tests subfolder to public?

Should this always be imported into the default view or can this be imported through the test plugin?

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

All tests pass within a couple seconds. ⏱

@RobAndrewHurst
Copy link
Contributor Author

I don't fully understand the public/_mapp.test.js library.

Should this sit in a tests subfolder in js since there is already a tests subfolder to public?

Should this always be imported into the default view or can this be imported through the test plugin?

Agreed. I will move the _mapp.test.js module.

The main reason for the tests directory in public was because some scripts needed to be made accessible by the public directory. This isn't the case anymore since we have a bundled asset that gets shipped. These will be removed in prs to come :)

@RobAndrewHurst RobAndrewHurst merged commit 0e63528 into GEOLYTIX:minor Dec 18, 2024
7 checks passed
@RobAndrewHurst RobAndrewHurst deleted the test-plugin branch January 14, 2025 12:18
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.

Test Clean up and Performance changes.
3 participants