-
Notifications
You must be signed in to change notification settings - Fork 885
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
[OSCI][ADD] More tests for overview #5418
[OSCI][ADD] More tests for overview #5418
Conversation
Signed-off-by: Thanh Le <lechithanh2003@gmail.com>
Signed-off-by: Thanh Le <lechithanh2003@gmail.com>
Signed-off-by: Thanh Le <lechithanh2003@gmail.com>
Signed-off-by: Thanh Le <lechithanh2003@gmail.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5418 +/- ##
===========================================
- Coverage 66.97% 55.68% -11.30%
===========================================
Files 3293 3005 -288
Lines 63289 59323 -3966
Branches 10065 9453 -612
===========================================
- Hits 42390 33032 -9358
- Misses 18458 24235 +5777
+ Partials 2441 2056 -385
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thanhinhchtom Thanks for adding these test cases to increase our coverage in this plugin - it's certainly better to have snapshot tests rather than no tests at all. But in a follow-up issue/PR, I think we can continue to improve these tests by actually validating/asserting the intended behavior instead of just capturing snapshots.
While snapshot tests are by far the easiest to write, they have a couple disadvantages compared to making more specific assertions:
- They're brittle, because they often capture the specific implementation of the component, not just its business logic. That means that almost any change to the component code will generate a snapshot diff, which then has to be analyzed by the contributor and reviewers manually to verify that all the changes are intended/desired.
- They don't tell you what the desired/important behavior is - in fact, they don't even guarantee that the current implementation works as intended.
newsFetchResult: null, | ||
}; | ||
const component = shallowWithIntl(<Overview {...props} />); | ||
expect(component).toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to re-write as a unit test, we'd want to assert that:
- The
<NewsFeed>
component is not rendered - The
.osdOverviewMore
section is rendered (solutions) - The
.osdOverviewData
section is not rendered (solutions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth repurposing this to ensure that newsfeed is not returned? or just delete in the same line as the tutorials.
I think originally we left it in because we could have re-used it. But now it just feels like bloat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuarrrr So should I rewrite those tests to finish this issue or this one will be merged as it is now and I will create a new one and fix it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thanhinhchtom From my perspective, this is an iterative improvement, and I'd be in favor of merging as-is with the follow-up improvements tracked in a separate issue (and subsequent PR). But other maintainers may have other perspectives.
Signed-off-by: Josh Romero <rmerqg@amazon.com>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-1.3 1.3
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-1.3
# Create a new branch
git switch --create backport/backport-5418-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9ba5cf54576724f48afde224c90d83ba504608ea
# Push it to GitHub
git push --set-upstream origin backport/backport-5418-to-1.3
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-1.3 Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5418-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9ba5cf54576724f48afde224c90d83ba504608ea
# Push it to GitHub
git push --set-upstream origin backport/backport-5418-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x Then, create a pull request where the |
* add more tests for overview * add more tests for overview * add to CHANGELOG * change log --------- Signed-off-by: Thanh Le <lechithanh2003@gmail.com> Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 9ba5cf5)
* add more tests for overview * add more tests for overview * add to CHANGELOG * change log --------- Signed-off-by: Thanh Le <lechithanh2003@gmail.com> Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 9ba5cf5)
Manual backport for 2.x - #5769 |
* add more tests for overview * add more tests for overview * add to CHANGELOG * change log --------- Signed-off-by: Thanh Le <lechithanh2003@gmail.com> Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 9ba5cf5) Co-authored-by: Thanh Le <lechithanh2003@gmail.com>
Description
Add some tests for other complications of overview part
Issues Resolved
fixes #4693
Screenshot
Testing the changes
yarn test:jest src/plugins/opensearch_dashboards_overview/public/components/overview/overview.test.tsx
Check List
yarn test:jest
yarn test:jest_integration