-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Unify session loading using implicit-hie #1783
Conversation
@emilypi Would you mind trying if this fixes your issue? |
1730357
to
73b472a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know that this fixes the issue? It would be great to have a test
I think, we need end-to-end tests to truly prove the issue doesn't occur anymore. The issue was basically that Wrapper and HLS didn't use the same logic for loading a cradle, thus, came to different conclusions regarding the cradle type. As we now unified the logic behaviour, e.g. there is only a single occurence of Do we have something resembling end-to-end tests? |
Execute hls-wrapper and hls in a test directory as cli and comparing their outputs about loading the cradle could work? |
@fendor I can confirm this works, calling the executable with HEAD on your branch:
|
Make Wrapper and Session loader use the same logic to avoid loading logic divergence. Cleans up existing usages to use infrastructure in place.
faebd77
to
3b34b92
Compare
Added a test as suggested by @jneira Implements new major mode I had troubles running the tests locally, let's see what CI says. |
565374a
to
59297ea
Compare
Adds test-case for proving that wrapper and hls report the same cradle type for a project.
As per usual, you spend more time on trying to write tests than actually solving the issue. I think this is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great job @fendor, as usual
I hope the new cli option will be helpful beyond this concrete issue.
Make Wrapper and Session loader use the same logic to avoid
loading logic divergence.
Cleans up existing usages to use infrastructure in place.
Closes #1782