-
Notifications
You must be signed in to change notification settings - Fork 48
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
issue #4494 Allow endpoint handlers configurations from inside a test #4952
Closed
rrrooommmaaa
wants to merge
1
commit into
master
from
issue-4494-allow-endpoint-configuration-from-test
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
I didn't mean this way, setting handlers.
I meant we would be setting configuration. Since now we have one MockApi per per test, we have an opportunity to configure mock api with different data (or expectations).
Like this:
notice it's receiving information on what to return (per service), not receiving the endpoints directly as done in the PR.
Have a look at any iOS mock tests, like these:
https://github.com/FlowCrypt/flowcrypt-ios/blob/master/appium/tests/specs/mock/composeEmail/CheckInvalidEmailRecipient.spec.ts
https://github.com/FlowCrypt/flowcrypt-ios/blob/master/appium/tests/specs/mock/composeEmail/CheckSendAsAlias.spec.ts
Notice how each test has some unique mock api configuration to work with, so it's clear which data the test needs.
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.
My suggestion doesn't rule out a configuration structure. It's just more straightforward to define endpoints in this way, for example:
Thus we see what endpoints are expected to be used in this test
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.
As for me, it'll just make tests setup more complicated if we'll need to set handlers separately for each test, while it won't give us any advantages in tests configuration.
While setting custom
fesConfig
,ekmConfig
etc will allow us to have fully independent tests with different configs. They also can be updated during the test like this: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.
Agree with @sosnovsky in that @rrrooommmaaa 's approach seems more complex to set up for each test, without clear advantage compared to what was in the issue. Yes, it would allow to fine-tune which exact endpoints are allowed for the test, which would be more precise. But I don't think we need to go that far, and for tests that truly intend to test that, using @sosnovsky 's approach also doesn't mean we can't have a configuration to disallow certain exact endpoints for that test.
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.
It's up to you to decide.
However, as of PR #4928,
mockApi
isn't exposed to the test definition, or am I missing something?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.
@sosnovsky
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.
Yes, currently it's not exposed, I was planning to add it to
AvaContext
:(here - https://github.com/FlowCrypt/flowcrypt-browser/blob/master/test/source/test.ts#L68)
And then use it in tests like this: