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

WebtoolsMapsContext has been duplicated, leaving actual used context untested #112

Open
pfrenssen opened this issue Nov 6, 2019 · 5 comments · May be fixed by #113
Open

WebtoolsMapsContext has been duplicated, leaving actual used context untested #112

pfrenssen opened this issue Nov 6, 2019 · 5 comments · May be fixed by #113

Comments

@pfrenssen
Copy link
Member

For some reason the WebtoolsMapsContext has been duplicated in #94, removing the test coverage that was in place for the actual subcontext that is in use in existing sites.

This change was out of scope for that PR and is not motivated in any of the comments made. Let's revert this change to not disrupt test coverage for the existing subcontext.

It can be decided to step away from the subcontext in the module folder and move it to a context in the tests/Behat folder but since this affects current projects it can only be done in a major release.

@brummbar
Copy link
Contributor

brummbar commented Nov 6, 2019

The reason is https://github.com/openeuropa/oe_webtools/pull/94/files#diff-b21fa0b75a72fac761e40d689e55338dR16 .
We don't use anymore the context provided in .behat.inc .

@pfrenssen
Copy link
Member Author

Yes I know this, this change in BDE was my proposal 😁

But this is still in use in existing sites, so we should still keep it and ensure it is covered by tests until we are ready for OE Webtools 2.x

@ademarco
Copy link
Member

ademarco commented Nov 6, 2019

Changing how we test our components does not constitute a change in the component's API and it does not motivates a new major release since sites will continue to work without any manual intervention. In this case tests will need to be ported to comply with the new structure.

@pfrenssen
Copy link
Member Author

OK I agree that we shouldn't waste too much energy on this. Maybe a simple solution could be to add a simple check to ensure that both files contain identical code. Then we can keep the test coverage on the main context.

We'll need to account for a few small differences like the module namespace and the disclaimer.

I'll try to give this a shot.

@pfrenssen
Copy link
Member Author

pfrenssen commented Nov 6, 2019

In #113 I have proposed a simple check to ensure that the contents of both contexts are identical. To account for the differences in the file headers (the docblocks and namespaces have some differences) I am only checking the lines inside the class. This is done by using a simple diff command and checking if this returns an error code.

I have added this is a small shell script. I first tried to put it directly in .drone.yml but it did not agree with using too much shell magic in it. I also tried to put it as a command in runner.yml.dist but I could not figure out how to correctly escape it.

I figured in the end that a shell script is probably the easiest solution, once we reach 2.x we can just remove the deprecated subcontext and the shell script and we're done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants