This repository has been archived by the owner on Dec 30, 2022. It is now read-only.
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.
test: add End-2-End test suite #2824
test: add End-2-End test suite #2824
Changes from all commits
206a04d
7ee4668
7b7c1c5
0d10190
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think I asked this question already but I'm no longer sure what's the answer.
Is it not possible to hide this away in
instantsearch-e2e-tests
and just expose a command there?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.
See my answer: algolia/instantsearch#4035 (comment)
(it can be discussed of course 😉)
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 also think we should hide 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.
It would also make sense to me.
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.
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.
Well... I'm not for this.
First of all there is very few advantages (if any?) to "hide" dependencies but the real problem is mainly that it will makes us call a subdependency from the parent project (
react-instantsearch
callswdio
directly), which is not recommenced (the package version we're targeting may not be at thenode_modules
top level, depending how npm/yarn deduplicated the dependencies).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.
can we also hide
wdio
call then?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.
What would be the objective of hiding these dependencies?
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 would remove the overall perceived complexity, as well as reduce the noise and cognitive overhead of managing these dependencies.
I find it surprising that we went all the way to packaging the E2E tests (e.g. see also
wdio.saucelabs.conf.js
which only exports the E2E package import), but are reluctant to hide the dependencies themself.The E2E test suite would be more predictable if the
wdio
dependencies are packaged within it. Here, we are responsible for thewdio
deps to work with a black-boxed test suite, which is quite contradictory.