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

Check that the old and new webtools_maps contexts are identical. #113

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pfrenssen
Copy link
Member

Description

For Webtools Maps we currently maintain both the deprecated OeWebtoolsMapsSubcontext and the new WebtoolsMapsContext. Both are supposed to contain identical code, but we have no checks in place to ensure this. The tests only cover the new context.

There appears to be a small divergence already.

Fixes #112

Change log

  • Added: Automated check to ensure both Behat contexts for Webtools Maps remain synchronised.

@pfrenssen
Copy link
Member Author

The last test is working as expected: the deprecated subcontext has diverged from the new version, and the test indicates which lines are not correct:

+ ./tests/scripts/validate-subcontext-integrity.sh
52c52
<       Assert::assertNotEmpty($maps);
---
>       Assert::assertNotEmpty($this->getWebtoolsMaps());

I will now commit a fix that will make both files identical again, this should (hopefully) turn the build green again.

@ademarco
Copy link
Member

ademarco commented Nov 8, 2019

Haven't had the time to look into this properly, so I might shoot in the dark here, but instead of ensuring that two classes have the same code can't we deprecate the old one (with @deprecated ...) and have it extend the new one instead? We use composer autoloading anyway, the new one will be available anyway.

@pfrenssen
Copy link
Member Author

@ademarco this is an interesting idea, but it might be difficult since both classes extend a different base class. But we can provide a trait or maybe a different base class with the common methods in it? I can look into this.

It would make it more work though to remove the deprecated code in the future, and we would then have to deprecate the trait / base class in the future 🤔

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 this pull request may close these issues.

WebtoolsMapsContext has been duplicated, leaving actual used context untested
2 participants