Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow removal of platform instance for testing purposes #41

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

Merged
paulppn merged 3 commits into Baseflow:main from JeroenWeener:main
Mar 23, 2023

Conversation

Copy link
Contributor

@JeroenWeener JeroenWeener commented Mar 14, 2023

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Adds method to platform interface to remove the registered instance. This is useful for tests where a test needs to check the behavior of a non-existing instance.

⤵️ What is the current behavior?

There is no way to set the instance to null, as there is a specific check to prevent users from doing that.

🆕 What is the new behavior (if this is a feature change)?

Using removeInstance, tests can unregistered a previously registered instance of the platform interface.

💥 Does this PR introduce a breaking change?

No.

🐛 Recommendations for testing

📝 Links to relevant issues/docs

This PR is part of #33.

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated
  • Rebased onto current develop

Copy link

@paulppn paulppn left a comment

Choose a reason for hiding this comment

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

LGTM

@paulppn paulppn merged commit 7e5c85e into Baseflow:main Mar 23, 2023
/// This method is exposed for testing purposes only and should not be used by
/// clients of the plugin.
@visibleForTesting
static removeInstance() => _instance = null;
Copy link
Member

@mvanbeusekom mvanbeusekom Mar 24, 2023

Choose a reason for hiding this comment

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

Is there a specific use case for this?

I have not seen it in any of the other plugins and from the top of my head I cannot really think of a specific case where this is needed. Would like to hear if there is one.

Copy link
Contributor Author

@JeroenWeener JeroenWeener Mar 24, 2023

Choose a reason for hiding this comment

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

It is included so we can test whether the methods that are exposed to users of the plugin correctly throw if no platform is registered. This way we can emulate the scenario where the app runs on an unsupported platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvanbeusekom mvanbeusekom mvanbeusekom left review comments

+1 more reviewer

@paulppn paulppn paulppn approved these changes

Reviewers whose approvals may not affect merge requirements
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /