search

matrix-org / matrix-react-end-to-end-tests

End to end tests for the matrix react web app
2 stars 2 forks source link

Consider merging into matrix-react-sdk #44

Open jryans opened 5 years ago

jryans commented 5 years ago

I am curious why this exists as a separate repo from matrix-react-sdk. Since the most frequently changing components covered by these integration tests are in matrix-react-sdk, would it be simpler to also locate these tests in some subdirectory of the matrix-react-sdk repo? This way the React components and any affected tests can be updated in the same PR. It would also avoid the need to sync branches when running the tests (as in https://github.com/matrix-org/matrix-react-sdk/pull/2387), which is sometimes forgotten for specialized work.

Paging @matrix-org/riot-web to request opinions one way or another from the team.

turt2live commented 5 years ago

I'd kinda want to keep them separate to avoid pulling in largish dependencies which go unused for day-to-day work.

jryans commented 5 years ago

I'd kinda want to keep them separate to avoid pulling in largish dependencies which go unused for day-to-day work.

It wouldn't necessarily have to impact day to day work. (I am assuming your main concern would be if the dependencies here were added into the devDependencies of the React SDK, forcing all devs to install them.)

We wouldn't necessarily need to do it that way. For example, this could be merged into some subdirectory of the React SDK and have things set up to only install the deps when explicitly running the tests (so the primary list of devDependencies for the SDK itself would not be changed).

jryans commented 5 years ago

(I should add that personally I find it nice to have these tests installed locally so that I can reproduce test results when needed, which means I already use them in day-to-day work. This sounds like a different approach from the one @turt2live currently uses.)

turt2live commented 5 years ago

I run the tests I can, but don't run these tests for a few reasons: I use a Windows environment and can't run these tests, the test coverage is limited, and the tests take quite a while to run. I generally prefer to rely on the CI to run the tests for me as by the time I open the PR I'm reasonably sure the tests will pass anyways.

The other thing to consider is contributors probably shouldn't have to pull down all these dependencies either. Setting up a riot-web development environment is hard enough as-is, and the extra N minutes to install dependencies they also probably won't use feels wrong to me.

turt2live commented 5 years ago

As per the retro: I think we should put these in the riot-web repo. They're more Riot end to end tests rather than react, even if the meat of the app is in the react-sdk.

jryans commented 5 years ago

The downside of merging to riot-web is that it's not much better than leaving them where they are now in this wholly separate repo, since you're more often than not are still having to change 2 repos (since your code change is likely React SDK).

(At least in riot-web, they would get versioned together with app being tested, though...)

turt2live commented 5 years ago

This might also be a good time to open up the can of worms labeled "merge riot-web and react-sdk together" again. With the foundation underway, it's arguably more important that we have a clear separation between New Vector and Matrix, which includes Riot from Matrix.

bwindels commented 5 years ago

This might also be a good time to open up the can of worms labeled "merge riot-web and react-sdk together" again. With the foundation underway, it's arguably more important that we have a clear separation between New Vector and Matrix, which includes Riot from Matrix.

I'm assuming that any Riot repos would end up under vector-im? Either merged into riot-web (see below) or moving the repos to the vector-im org.

I see the point that e2e tests, by definition, test the app as a whole, not an SDK. But in practice, I think it will increase the pain (having to create multiple PRs) of having multiple repos, which I think we're sort of stuck with for now.

We've discussed merging a few times. Last time, IIRC, the conclusion seemed to be let's keep it as it is because at some point we'll need a proper SDK and merging the repos gets us further from that.

bwindels commented 5 years ago

so, what do we do?

jryans commented 5 years ago

I am assuming not much has changed on the merging / separate repos story, so I assume we're continuing with separate repos for now...?

bwindels commented 5 years ago

Or we merge them into riot-web. They'll need a complete riot in any case to run, so if we put them in the react sdk, we'll have to cd ../riot-web somewhere which is also not ideal ...