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.

Sign up for GitHub

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

Flaky QuickDebugSourceInstallingCompilationParticipantTest #3354

Merged
merged 5 commits into from
Feb 21, 2025
+6 −8
Merged

Flaky QuickDebugSourceInstallingCompilationParticipantTest #3354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94e4960
use ClassReader(InputStream)
LorenzoBettini Feb 17, 2025
a04df68
avoid NPE when it has to fail
LorenzoBettini Feb 17, 2025
9e08f2e
getModificationStamp instead of getLocalTimeStamp
cdietrich Feb 19, 2025
7cc6b0d
renamed getModificationstamp
LorenzoBettini Feb 21, 2025
cb7a990
Apply suggestions from code review
LorenzoBettini Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions ...clipse/xtend/ide/tests/compiler/QuickDebugSourceInstallingCompilationParticipantTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.Opcodes;

import com.google.common.io.ByteStreams;
import com.google.inject.Inject;

/**
Expand Down Expand Up @@ -66,12 +65,11 @@ def dosomething() {
final AtomicBoolean debugInfoFound = new AtomicBoolean(false);
Path classFile = clazz.getLocation().toPath();
try (var in = Files.newInputStream(classFile)) {
final byte[] bytes = ByteStreams.toByteArray(in);
final ClassReader r = new ClassReader(bytes);
final ClassReader r = new ClassReader(in);
r.accept(new ClassVisitor(Opcodes.ASM9) {
@Override
public void visitSource(final String source, final String debug) {
if ("Outer.java".equals(source)) {
if ("Outer.java".equals(source) && debug != null) {
assertEquals("""
SMAP
Outer.java
Expand Down
4 changes: 2 additions & 2 deletions org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/generator/trace/TraceForStorageProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ public AbsoluteURI getPath() {
}

@Override
public long getTimestamp() {
return file.getLocalTimeStamp();
public long getModificationStamp() {
return file.getModificationStamp();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying assumption seems to be that the code executes too fast.
Would it fail if we wait for one second in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szarnekow I haven't tried, but the Javadoc of getLocalTimeStamp, https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fapi%2Forg%2Feclipse%2Fcore%2Fresources%2FIResource.html&anchor=getLocalTimeStamp(), also says

Note that due to varying file system timing granularities, this value is not guaranteed to change every time the file is modified. For a more reliable indication of whether the file has changed, use getModificationStamp().

It sounds like the method is quite unreliable...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess a sleep of 1s would also help. but am not sure if other tests might be affected too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure.

@szarnekow what do you think is wrong with getModificationStamp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a timestamp and thus should not be used to implement getTimestamp

It might be fine to rename getTimestamp to getModification stamp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a timestamp and thus should not be used to implement getTimestamp

It might be fine to rename getTimestamp to getModification stamp

@szarnekow OK, I'll do that later.

That would be API breakage but I guess for that wouldn't be a problem because it's an internal type, would it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szarnekow renamed it (7cc6b0d)

}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...pse.xtext/src/org/eclipse/xtext/generator/trace/internal/AbstractTraceForURIProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public abstract class AbstractTraceForURIProvider<SomeFile, Trace extends Abstra

protected interface PersistedTrace {
AbsoluteURI getPath();
long getTimestamp();
long getModificationStamp();
InputStream openStream() throws IOException;
boolean exists();
}
Expand All @@ -69,7 +69,7 @@ private static class CacheKey {

public CacheKey(PersistedTrace source) {
this.path = source.getPath();
this.timestamp = source.getTimestamp();
this.timestamp = source.getModificationStamp();
}

@Override
Expand Down
Loading