[Notifications] onboard sanity tests for notifications plugin #185
Conversation
kavilla
force-pushed
the
avillk/onboard_notifactions
branch
from
2729623 to
6d5d059
Compare
New plugin. Just copy and pasted the existing tests from this repo: https://github.com/opensearch-project/notifications/tree/main/dashboards-notifications I am not positive about the flakiness this introduces but will handle it in subsequent tests. Issue: opensearch-project#184 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla
force-pushed
the
avillk/onboard_notifactions
branch
from
6d5d059 to
066b658
Compare
|
Can you add a PR check workflow for this plugin similar to other plugins? |
I think so! Do you manually create that from the workflow yaml file? The only thing is does it require an existing run? I'm not positive if there has been a successful run with OpenSearch that includes the notifications plugin. |
I created a template for such. Check this example. https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/.github/workflows/gantt-chart-release-e2e-workflow.yml If it cannot pass at this moment, it is fine to merge since at least it has the set up. |
Oh ha my |
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Added! It should fail though. |
|
It looks like all the other test workflows are checking out version 1.3.0 of OpenSearch. Are those going to be bumped to 2.0-rc1 as well? |
|
Also, is this PR copying all of the Cypress tests in the Notifications repo into this one? Can we not checkout the Notifications repo and run the Cypress tests that way? This is duplicating the code and if we change the tests on our side it wouldn't reflect here. By the way, the Cypress tests for Notifications are failing right now (as in they need to be updated due to changes in the frontend in preparation for release). For the time being, only the Jest unit tests are stable. |
Probably should but I will defer to @tianleh for that notifications plugin didn't exist before so couldn't use 1.3.0, am I correct?
You can but the idea here is just to get notifications plugin onboarded and tested against a full distribution with other tests, with sanity tests it can differ from the integration tests within the plugin repo. It was also have the added benefit running all tests for each plugin together so a true E2E test. Albeit there is redundancy here in this PR just coping and pasting but feel free to modify the tests post onboard so we know how this plugin impacts the rest of of the distribution. Another added benefit of onboarding here is that if there is any library breaking changes that come upstream you wouldn't have to worry about it as much as maintaining your own distribution. Where handle interactions with the security plugin as well. That said if you only rely on unit tests how confident are you in the actual plugin in a distribution? How will you verify it? Does the security plugin impact it? |
|
What I mean is, we keep the Cypress tests in our own repo and I'm pretty sure every other repo does too. They are usually run locally and as part of the workflow for the repo in the PR. If we are copying those tests here, then it's a duplication since we have them in our repo and they could change over time (we'll lose the benefit of the new tests added there being run here). If we remove them from the repo, then that puts our tests in two different places which I'm not a fan of. I think the long term solution should be to have the repo's checked out when the Cypress tests are run for each installed plugin (looks like this is the workflow to change for that?) so it's still run against the full distribution but there is a single source of truth for the test definition. I'm curious why we didn't do it that way from the beginning? Are there any issues with that approach that I'm not aware of?
For Notifications, the Cypress test broke as we were making changes and haven't had a chance to update them yet since we're trying to get ready for release. We don't plan on only relying on just Jest tests, I just wanted to let you know that if we run them for Notifications as they currently are, it might fail. |
I understand, I think just to be clear the point of copying the tests here is just so there is some tests running for the full distribution tests. These don't have to be the final draft of the tests but just something. If you'd prefer to manage your own you will need to do this for every release: https://github.com/opensearch-project/opensearch-build/blob/main/ONBOARDING.md#onboard-to-test-workflow. I think the confusion is that we should consider this repo as a sanity test repo for a full E2E experience. Compared to having each individual plugin ran individually in it's own controlled environment. Another issue is that once OpenSearch Dashboards onboards cypress as it's official test framework OpenSearch Dashboards plugins will now have conflicts since not all plugins are on the same version but will conflict with the dependency tree that it will inherit from OSD. All this coupled with us handling any issue with an node upgrade incompatibility, ensuring compatibility with ARM64, and handling security plugin workflows, it would be less work for your repo if onboard some sanity tests into this repo and have integrations test running per PR in your repo. But if you prefer then you will need to make sure for |
|
Let's merge and ask for plugin team to fix flake test |
|
What's the plan to merge since we need automation. Can we merge this PR and ask plugin owners to continue fix this? This would make us having integTest for all dashboards plugins. Thanks. |
Merging. As @qreshi mentioned above, there might be some flakeness about the tests. |
Description
New plugin. Just copy and pasted the existing tests from this repo:
https://github.com/opensearch-project/notifications/tree/main/dashboards-notifications
I am not positive about the flakiness this introduces but will handle it in subsequent tests.
Signed-off-by: Kawika Avilla kavilla414@gmail.com
Issues Resolved
#184
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.