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

Adopt memorystore for tests #745

Merged
merged 5 commits into from
Mar 5, 2024
Merged

Adopt memorystore for tests #745

merged 5 commits into from
Mar 5, 2024

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Feb 16, 2024

Summary

This PR adopts the MemoryStore (mattermost/mattermost#26244) from the tests in server/plugin/subscriptions_test.go and removes the mocks.

I've included a cleanup of p.API usage via 3f0f4fc in this PR.

Ticket Link

None

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Feb 16, 2024
@hanzei
Copy link
Contributor Author

hanzei commented Mar 1, 2024

@mickmister @raghavaggarwal2308 Gentle reminder to review the PR

Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 left a comment

Choose a reason for hiding this comment

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

@hanzei LGTM 👍🏽

Note: tests are failing in CI

@hanzei
Copy link
Contributor Author

hanzei commented Mar 1, 2024

I've reached out to @ayusht2810 regarding the failing CI in https://community.mattermost.com/core/pl/z7833z1f6iyaibmib4af13fzar

@mickmister mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Mar 1, 2024
@hanzei hanzei added this to the v2.2.0 milestone Mar 1, 2024
@raghavaggarwal2308
Copy link
Contributor

I've reached out to @ayusht2810 regarding the failing CI in https://community.mattermost.com/core/pl/z7833z1f6iyaibmib4af13fzar

@hanzei The tests are working fine on my local for master and your branch as well. Can you please try re-running the CI here?

@hanzei
Copy link
Contributor Author

hanzei commented Mar 5, 2024

@raghavaggarwal2308 I've re-triggered CI, which fixed the issue.

Please note that the tests on master also fail. It seems like we have a flaky test there.

@hanzei hanzei merged commit aa1faef into master Mar 5, 2024
9 checks passed
@hanzei hanzei deleted the memorystore branch March 5, 2024 11:49
@hanzei
Copy link
Contributor Author

hanzei commented Mar 5, 2024

Ok, master is also green now. Seems like the issue is fixed 🤷

@hanzei
Copy link
Contributor Author

hanzei commented Mar 5, 2024

@raghavaggarwal2308 @mickmister Do you think adopting the memory store would be helpful for other plugins that extensively make use of the KV store like Gitlab or JIRA?

@hanzei
Copy link
Contributor Author

hanzei commented Mar 6, 2024

@Kshitij-Katiyar
Copy link
Contributor

@raghavaggarwal2308 The nightly run failed again: https://github.com/mattermost/mattermost-plugin-github/actions/runs/8164766247/job/22320677668

@hanzei I tried running the commands locally many times but was not really able to reproduce this.
Screenshot from 2024-03-19 18-49-21

@hanzei
Copy link
Contributor Author

hanzei commented Mar 19, 2024

My own tests were the ones that were flaky. 🤦 Submitted mattermost/mattermost-plugin-starter-template#197.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants