Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clear cache on reload #11673
Clear cache on reload #11673
Changes from all commits
5266a4b
d803b28
38a20b4
0f437fc
404e3cc
5629e20
b1d4b37
0e42e9d
0b4e1f8
f5f1614
dad2bb6
ada146e
e26a868
da895d8
96ea1f2
e003b8a
5d97a2a
6a1e89f
c493ec0
131c52e
4f750b6
5b563d4
dc2decd
1fb2517
ef15d90
0738190
5f176e7
85e8b6d
2e9d0c3
46ef5fb
e56b867
3ca29aa
da2d438
ae83008
276a885
205811c
40aedfc
2e91135
786f156
934ec5f
a3c07e0
a7890d2
7c3c33c
20b14b5
375fb31
c9ee885
00ccaca
2899524
eeba1dc
afa0dfc
4f22fe5
0aa2490
2b01606
a041045
9b34a56
1f13014
8ca2e23
eebe998
f168299
95208ac
9796234
07f413d
e6ad441
c2c1ada
828ac2d
be50dcd
d036b8b
5aa4003
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm not convinced about this mock.
After all - tests relying on this mock use completely different logic for checking if the cache should be cleared than what will be running in the IDE. The
Reload_Detector
is a new component and it is not tested anywhere at all (apart from manual testing).I think it would be best to add a test to the JVM tests, similar to something like
RuntimeExecutionEnvironmentTest
that checks the refresh logic is correctly connected through the Language Server. Perhaps we don't need to test it all the way down to HTTP. It would probably suffice to have a test that instantiates the JavaReloadDetector
and checks that it indeed detects cache invalidation correctly.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.
I think perhaps we may want to proceed with this PR, as it's needed for release, with just manual testing. But I think it would be good to have some test that ensures that this works as expected.
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.
Logic of
Managed_Resource.register ref on_finalize Boolean.True
is unit tested atenso/engine/runtime-integration-tests/src/test/java/org/enso/interpreter/runtime/ManagedResourceTest.java
Line 96 in cc0b020
If we have a unit test of
Managed_Resource
and another unit test forReloadDetector
, then I believe we have 100% coverage!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.
My point was that we are not testing the integration between
ReloadDetector
andManaged_Resource
logics if we rely on a mock when testingReloadDetector
.The problem is clear in the
get
bug which if I understand correctly you agree with me is a bug - we are not catching the possibleUninitialized_State
there, possibly returning dataflow error instead of boolean. This is not covered by any existing test as far as I can tell. Given that we seem to have a bug and no test seems to be catching the bug - I'd say we don't have good enough tests for this facility.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.
I agree that a test like this is necessary -- precisely because I have already had the tests pass but the feature fail to work. Thus: #11792.