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

Refactor runtime testsuite #3367

Closed
KvanTTT opened this issue Nov 28, 2021 · 10 comments
Closed

Refactor runtime testsuite #3367

KvanTTT opened this issue Nov 28, 2021 · 10 comments

Comments

@KvanTTT
Copy link
Member

KvanTTT commented Nov 28, 2021

There is a lot of duplicated code in runtime test classes that can be extracted to a common abstract class. This issue can be assigned this issue to me.

@ericvergnaud
Copy link
Contributor

I wouldn't be too worried about this.
I actually did the work some time ago and then dropped it, because I suspect it could on the long run create problems. Each target has specific requirements, and having the flexibility of changing the tests behavior for a specific target without having to think about other targets is highly valuable, especially given the specific skills of target authors.

@KvanTTT
Copy link
Member Author

KvanTTT commented Nov 29, 2021

I don't completely agree. Most part of logic is common, specific parts can be extracted to specific classes. Also, it seems like some logic is excess.

I suspect it could on the long run create problems

If all tests are green after refactoring then there are no problems. Why type of problems do you mean?

Each target has specific requirements, and having the flexibility of changing the tests behavior for a specific target without having to think about other targets is highly valuable, especially given the specific skills of target authors.

As I've already said, specific logic can be successfully extracted to concrete classes, it's not a problem. Flexibility remains at the same level. I think we (maintainers and active contributors) shouldn't rely on target authors because they may suddenly leave the project. Moreover, they also may not know about all details (for instance, about optimizations in my latest PR).

Also, I would say the new structure of the tests suite will make it easier to add new targets and reduce the number of errors due to duplication.

@parrt
Copy link
Member

parrt commented Dec 9, 2021

per discussion #3369 easiest/least intrusive change is to use text blocks and convert comments with values into actual strings; tweaking the test classes (descriptors).

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 9, 2021

It's about code itself, not about test data. It will allow us to add tests for new targets more easily and make code more clear. There is a lot of duplication now.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

Resolved with #3407

@parrt parrt closed this as completed Dec 23, 2021
@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 23, 2021

Actually not resolved. The issue is about something else, about code base, not about test data. Also, merge request is about data transformation, not refactoring.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

Ah I see. Well, I'm going to close as the test rig works and there's little advantage to refactoring at this point. If we dramatically expand that area then it would be time to Refactor I would say.

@KvanTTT
Copy link
Member Author

KvanTTT commented Dec 23, 2021

If we dramatically expand that area then it would be time to Refactor I would say.

It will allow adding new targets (Rust, maybe Kotlin) much easier. And will simplify code support.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

Yes, that is true that adding new targets would be easier, but one of my rules for software development is to program for today and not tomorrow ;) we may never add a new target for example.

@parrt
Copy link
Member

parrt commented Dec 23, 2021

Change today is not less expensive than a change later when it's actually needed

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

No branches or pull requests

3 participants