search

thSoft / elysium

LilyPond IDE for Eclipse
http://elysium.thsoft.hu
14 stars 3 forks source link

#197 Upgrade dependencies #198

Closed thSoft closed 3 years ago

thSoft commented 3 years ago

Fixes #197.

thSoft commented 3 years ago

Unfortunately there is a regression in the integration tests: https://travis-ci.org/github/thSoft/elysium/jobs/754998921#L2714 Could you please investigate it, @nittka?

nittka commented 3 years ago

Yes, I'll have a look. I just checked that the tests still run successfully in my old Eclipse development environment. I'll first have to setup an updated Eclipse using the setup from the branch to check what might be the problem with the latest Xtext.

thSoft commented 3 years ago

Great, thanks! Use version 2020-12 for both Eclipse application and target platform during setup (I'll update the Contributor Guide wiki page when this PR is merged to master).

nittka commented 3 years ago

I guess, investigating this problem might take a bit. While the variable is marked as unknown when running the unit test, it can be resolved in a runtime workspace (at least there are no errors and F3 finds a definition). I am not aware that we modified in the actual linking in the UI-Module.

Was removing the caching of default imports for tests necessary? The execution time for the integration tests is much higher than in the "old" verison.

thSoft commented 3 years ago

Was removing the caching of default imports for tests necessary? The execution time for the integration tests is much higher than in the "old" verison.

No, thanks for pointing this out. It was just because the Xtext testing API changed significantly and I was lazy to migrate the manual injection code. :) I'd gladly migrate it, but unfortunately I couldn't find any migration guide. Do you have any pointer?

nittka commented 3 years ago

OK, the problem is almost certainly caused by the simplifying changes in the Integration test itself. Now the parsehelper only gets the file content but not the URI (previously the file URI was explicitly set), so a synthetic URI is created. But this URI cannot be used to resolve an included file (no base URI for resolution). For default includes this is irrelevant because they are absolute... This also explains why the test fails while there are no errors in the runtime workspace.

The problem can be reproduced by a trivial test case (ily defines a variable, ly includes and references the variable). I will try to update the tests and also reintroduce the cache if possible. I don't know of a migration guide and have not intensively worked with Xtext quite some time.

nittka commented 3 years ago

I did not want to modify this branch directly, so I created a PR for adding a fixing commit to it. Thanks for the effort of updating. Currently, we use no new Xtext features, so requiring Xtext 2.24 could by quite restricting. But I guess whoever is using Elysium is not really bound to an old version...

thSoft commented 3 years ago

OK, the problem is almost certainly caused by the simplifying changes in the Integration test itself.

Sorry for breaking the test. :( Thank you for fixing regression caused by me!

Currently, we use no new Xtext features, so requiring Xtext 2.24 could by quite restricting.

I know, I just wanted to upgrade the dependencies for good since they were quite old.