Skip to content

500, adding entry point to inspect command#528

Open
ErnestMatskevich wants to merge 30 commits into
objectionary:masterfrom
ErnestMatskevich:500
Open

500, adding entry point to inspect command#528
ErnestMatskevich wants to merge 30 commits into
objectionary:masterfrom
ErnestMatskevich:500

Conversation

@ErnestMatskevich

Copy link
Copy Markdown

Renamed branch to #500 for tracking issue
Added test for Inspect.java file

ErnestMatskevich and others added 6 commits May 21, 2025 02:29
Added:
qulice plugin in pom.xml
test_inspect.js file with unit test

For testing installed in dev dependencies:
sinon
proxyquire
add stronger signatire for main() in Inspect.java

add test for Inspect.java
@ErnestMatskevich

Copy link
Copy Markdown
Author

I added simple test for Inspect.java file, but when run qulice I faced with tha same warnings as in previous PR:

[WARNING] Unused declared dependencies found:
org.eolang:eo-runtime:jar:0.56.2:compile
org.junit.jupiter:junit-jupiter-api:jar:5.12.2:compile
org.junit.jupiter:junit-jupiter-engine:jar:5.12.2:compile
org.takes:takes:jar:1.24.6:compile

Also I would like to clarify how to build inspect.jar with Maven. Since currently there is single pom.xml file, Maven makes eoc.jar file as it writen in its . For complete my task I need to create my own file inspect.jar. I would like to use <execution> in pom.xml to separate creating eoc.jar and my inspect.jar. Is it correct idea? I think that after separate creating inspect.jar there will be no more warning about unused dependencies, because qulice will be placed in <execution> of inspect part and will not see junit and eo-runtime dependecies. @yegor256 What do you think about use in pom.xml?

There is 1 falling check in eslint workflow Is it on my side? If it is how can I fix that?

@yegor256

Copy link
Copy Markdown
Member

@ErnestMatskevich I fixed eslint, please merge master into your branch

@yegor256

Copy link
Copy Markdown
Member

@ErnestMatskevich show what you are trying to do and show the error (in CI logs). We'll be able to provide help.

@ErnestMatskevich

Copy link
Copy Markdown
Author

@yegor256 I merged master, now all cheks pass. I will try to edit pom.xml and show if any errors occurs in CI

Added profile section for creating independing inspect.jar file. Placed Takes and qulice inside profile
@ErnestMatskevich

Copy link
Copy Markdown
Author

@yegor256 I made profile in pom.xml for creating inspect.jar, all cheks passed. Please, take a look, is it good solution?

I have some questions:

  1. Should I create fat-jar (with takes dependency in inspect.jar)?
  2. Why org.junit.jupiter:junit-jupiter-api and org.junit.jupiter:junit-jupiter-engine have scope compile? As I know they are need for testing

@ErnestMatskevich

Copy link
Copy Markdown
Author

@yegor256 please take a look at my last commit in this PR. If there is no problem, let's merge? or close this PR and I will continue open new PR's with adding other functionality of inspect command. Moreover, I still interesing in questions wrote upper. Could you please clarify for me details about fat jar and scope of junit.

@yegor256

Copy link
Copy Markdown
Member

@ErnestMatskevich it's better to ask questions as separate tickets, see: https://www.yegor256.com/2025/05/25/bug-driven-development.html

@yegor256

Copy link
Copy Markdown
Member

@volodya-lombrozo can you please check this design?

@yegor256 yegor256 requested a review from volodya-lombrozo May 28, 2025 03:28

@volodya-lombrozo volodya-lombrozo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich

Thank you for the contribution! I have a few important notes related to the PR itself. Highly recommend reading "Helping others review your changes". Pay attention to the Write small pull requests and the Provide context and guidance sections. Could you also please make the title of this PR a bit more meaningful, rather than just "500", please?

@volodya-lombrozo volodya-lombrozo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yegor256

inspect -> debug

As I understand it, you want to write a debugger. Why not call it eoc debug then? Why should we hide this information under the misleading inspect command?

Separate Project

Why do we need to implement the entire debugger in the eoc repository? Maybe it's better to move it to a separate repo, as other projects do?
Here, we can just leave a thin eoc debug command just to invoke this debugger.
Moreover, eoc is written in JS. Adding Java would create a some sort of mess.

What do you think?

@yegor256

Copy link
Copy Markdown
Member

@volodya-lombrozo how would you call a tool that allows you go into a working Java virtual machine, find any object and see its details? Would it be a debugger?

Indeed, this tool deserves a separate project/repository, but maybe a bit later, when its first version is ready. So far, it's an early draft.

@volodya-lombrozo

volodya-lombrozo commented May 28, 2025

Copy link
Copy Markdown
Member

@volodya-lombrozo how would you call a tool that allows you go into a working Java virtual machine, find any object and see its details? Would it be a debugger?

Yes, it's what debuggers allow me to do - to "inspect" runtime.

@ErnestMatskevich ErnestMatskevich changed the title 500 500, adding entry point to inspect commands May 28, 2025
@ErnestMatskevich ErnestMatskevich changed the title 500, adding entry point to inspect commands 500, adding entry point to inspect command May 28, 2025
@ErnestMatskevich

Copy link
Copy Markdown
Author

@volodya-lombrozo Thank you for your review feedback. I've made the following improvements:

  1. Updated PR title to clearly state the functionality.
  2. Will follow up with smaller, focused PRs containing detailed descriptions

For the current changes in pom.xml added Maven profile section to build self-contained inspect.jar.

inspect command can be run with help

node src/eoc.js inspect 

or it is possible just build inspect.jar with help Maven and use -Pinspect for indepening package/compile.

Could you please review this profile configuration approach.

server.start();
Thread.sleep(2000L);
server.interrupt();
server.join(1000L);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Could you add clear assert statements, please? Here is a good paragraph how you can do it.

Comment thread mvnw/pom.xml Outdated
<build>
<finalName>eoc</finalName>
<sourceDirectory>${eo.generatedDir}</sourceDirectory>
<resources>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Why do we need to add this directory to the resources?

Comment thread mvnw/pom.xml Outdated
<finalName>inspect</finalName>
<plugins>
<plugin>
<groupId>com.qulice</groupId>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Why do we need to add qulice in this PR? It's a good tool, but it should be added in a dedicated PR.

Comment thread src/commands/inspect.js Outdated
module.exports = function(opts) {
return new Promise((resolve, reject) => {
const mvnDir = path.join(process.cwd(), 'mvnw');
console.info('Building EO inspect server with Maven...');

@volodya-lombrozo volodya-lombrozo May 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich @yegor256 Do we really need to build a java server each time we execute the eoc inspect command? It's suboptimal. Moreover, it requires Maven to be installed on the machine. And I haven't even mentioned the execution time yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@volodya-lombrozo @ErnestMatskevich indeed, it's better to build it once, when we package eoc. Then, simply run it.

Comment thread test/commands/test_inspect.js Outdated
const proxyquire = require('proxyquire');
describe('inspect command (mocked)', () => {
it('should send text and receive echoed response, then exit cleanly', async () => {
const mockStdout = new stream.PassThrough();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Can we simplify these variable names somehow? A Compound Name Is a Code Smell

ErnestMatskevich and others added 3 commits May 30, 2025 00:40
Added asserts in InspectSmokeTest.java and changed Inspect.java to handle interrupt
Delete qulice plugin and redundant resources section in pom.xml
@ErnestMatskevich

Copy link
Copy Markdown
Author

@volodya-lombrozo I fixed feedback items from your review. I splitted them into 2 commits:

  1. Fixed InspectTest.java at this commit
  • Renamed file to InspectTest.java
  • Wrote test with hamcrest
  • Rewrite from wait and see antipattern to adaptive waiting.
  1. Updated Inspect.java at this commit
  • Deleted unused import org.takes.http.Exit since now server could be interrupt
  • Simplify code with lambda. It looks clearly now and has same functionality
  • Removed redundant variable body

There is 1 check falling with building on windows, but macoc and ubuntu cheks passed. Is workflow falled because of my changes? The error in foreign part, but I did not edit it.

Could you please check my commits and help to fix build

Add maven assembly plugin which build inspect.jar

Refactor inspect.js - no more build inspect.jar, just run it

Refacror test_inspect.jar - no more testing maven part since it is not job of inspect command now
@ErnestMatskevich

Copy link
Copy Markdown
Author

@yegor256 @volodya-lombrozo I changed eoc inspect command behaviour. Now command mvn clean package creates inspect.jar with server and takes dependency inside. inspect.js just load inspect.jar and does not build it every time. Done in commit

I wrote descriptor src/assembly/inspect.xml for maven-assembly-plugin which I use for build inspect.jar and excluded .class files with my code in maven-jar-plugin for eoc.jar.

I deleted Maven part from inspect.js since inspect.jar is building by mvn clean package and deleted checks for Maven in test_inspect.js

There are problems in workflow with build and itests:

  • Build fails

  1. dataize at 0.56.2 version
  2. dataize with command-line argument
    dataization for older version is passed, but this 2 steps fails. Is problem on my side? I did not edit default building except adding exclusions for files with my code to avoid include them in eoc.jar
  • itests fails

Could't find object 'Φ.org.eolang.io'

Could you please take a look for my commit and help to fix build

@volodya-lombrozo volodya-lombrozo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich There are significant improvements. Thanks! Just a few small comments.

As for filling build, I suggest creating a new issue for that.

/**
* Private constructor.
*/
private Inspect() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Great thanks for the explanation. Now it's clear.

server.isAlive(),
is(true)
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Can we remove empty lines, please? It's a code smell.

}
assertThat(
"Server thread should be running after start",
server.isAlive(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Here you just check the the thread is alive. It tells nothing about the server availability.
https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#isAlive--

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@volodya-lombrozo Yes, the purpose of this test now is just check that server is starting without any errors. The availability of server is cheking in test_inspect.js file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich How do you know there aren't errors? It may be that the server didn't actually start, but the thread is still alive.

}
}
);
server.setDaemon(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich Why do we need this? From https://stackoverflow.com/a/17878439/10423604:

Any thread created by main thread, which runs main method in Java is by default non daemon because Thread inherits its daemon nature from the Thread which creates it i.e. parent Thread and since main thread is a non daemon thread, any other thread created from it will remain non-daemon until explicitly made daemon by calling setDaemon(true).

Remove empty lines as it code smell

Remove .setDaemon(false) as children thread is not daemon by default
@ErnestMatskevich

Copy link
Copy Markdown
Author

@volodya-lombrozo Thank you for your detail reviewes! I committed last changes.

In InspectTest.java:

  • Deleted empty lines
  • Deleted server.SetDaemon(false)

I opened issue for failing workflow

Could you please check last commit and help to understand reason of failing.

ErnestMatskevich and others added 3 commits June 22, 2025 20:48
Set src/main/java directly to sourceDirectory to show Maven right path to Java code.

Instead add <source>${eo.generatedDir}</source> to org.codehaus.mojo for opportunity run the tests
@ErnestMatskevich

Copy link
Copy Markdown
Author

Summary for commit

Updated PR to make it satisfactory with current branch and fixed some errors.

There is a bug in CI/CD: itest part and build

The problem is same for both steps and looks like:
Can not find "foo.bar.simple" object (java path is "EOfoo.EObar.EOsimple").

There are such files in generated-source folder, but they do not include in eoc.jar

Will try to solve it

Add new execution to place EOfoo package to classes and inside eoc.jar
Write cleaner exclude for ignoring only package-info.class for Java code of Inspector
@ErnestMatskevich

Copy link
Copy Markdown
Author

Fixed all errors

  1. Updated package-lock.json for includes node-fetch - commit

  2. Set src/main/java in <sourceDirectory> of <build> section in pom.xml for help Maven to see Java code of Inspector and avoid errors with unrecognising Java code, when run other commands which set different directories in <sourceDirectory>. For takes all previous functionality of other part of eoc source code, in build-helper-maven-plugin from pom.xml was set original <source>${eo.generatedDir}</source>. - commit

  3. The main problem was this line: <exclude>**/package-info.class</exclude> in pom.xml.
    The ** part exclude all package-info.class in project, but should excludes specific package-info.class for Inspect part.
    After refactoring line is: <exclude>org/eolang/package-info.class</exclude> and all checks are passed. - commit

@yegor256 Please, take a look

Comment thread src/commands/inspect.js Outdated
return new Promise((resolve, reject) => {
const mvnDir = path.join(process.cwd(), 'mvnw');
const jar = path.join(mvnDir, 'target', 'inspect.jar');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My bad, fixed in commit

@yegor256

Copy link
Copy Markdown
Member

@ErnestMatskevich looks good! I'm not sure though, it's a good idea to keep source files in the mvnw/ directory. Maybe we can create a new top-level directory called, for example, inspect/ and place src/main/java/ into it?

@yegor256

Copy link
Copy Markdown
Member

@volodya-lombrozo please, take a look one more time

@ErnestMatskevich

Copy link
Copy Markdown
Author

@ErnestMatskevich looks good! I'm not sure though, it's a good idea to keep source files in the mvnw/ directory. Maybe we can create a new top-level directory called, for example, inspect/ and place src/main/java/ into it?

@yegor256 I think while there is only Java code of inspect in project, no need to create new directory and leave Java files in standart Maven path (src/main/java). Moreover since you wrote that is is like a draft and should be placed in different repository in future, I think it is not a problem that for now files in mvnw.

@yegor256

Copy link
Copy Markdown
Member

@ErnestMatskevich we don't know for how long these files will stay with us. Once they are merged, they are with us (forever). It's better to place them correctly. The mvnw/ is not a "Maven folder". It's "Maven wrapper". The files you commit have nothing to do with the wrapper, right?

Extract source code files from mvnw and place them in inspect folder
@ErnestMatskevich

ErnestMatskevich commented Jun 23, 2025

Copy link
Copy Markdown
Author

@ErnestMatskevich we don't know for how long these files will stay with us. Once they are merged, they are with us (forever). It's better to place them correctly. The mvnw/ is not a "Maven folder". It's "Maven wrapper". The files you commit have nothing to do with the wrapper, right?

@yegor256 Done in this commit

Yes, I do not work with wrapper directly, i just build JAR file for server

@ErnestMatskevich

Copy link
Copy Markdown
Author

@volodya-lombrozo Please take a look. I added small commits described below. Also this commit about creating new top-level folder inspect with source code.

Fixed all errors

  1. Updated package-lock.json for includes node-fetch - commit
  2. Set src/main/java in <sourceDirectory> of <build> section in pom.xml for help Maven to see Java code of Inspector and avoid errors with unrecognising Java code, when run other commands which set different directories in <sourceDirectory>. For takes all previous functionality of other part of eoc source code, in build-helper-maven-plugin from pom.xml was set original <source>${eo.generatedDir}</source>. - commit
  3. The main problem was this line: <exclude>**/package-info.class</exclude> in pom.xml.
    The ** part exclude all package-info.class in project, but should excludes specific package-info.class for Inspect part.
    After refactoring line is: <exclude>org/eolang/package-info.class</exclude> and all checks are passed. - commit

@yegor256 Please, take a look

*
* @since 0.29.0
*/
final class InspectTest {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich It seems, that this test checks nothing. Moreover, it's too error prone. Can we just remove it?

Comment thread src/commands/inspect.js
}
console.log('Sending request with body:', input);
try {
const response = await fetch('http://localhost:8080/echo', {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich What if I already have another application running on 8080 port? Please, don't fix it here. Just leave a puzzle

Comment thread mvnw/pom.xml
</build>
<profiles>
<profile>
<id>inspect</id>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich I can't find the place in the code where you use this Maven profile. Can you give me a link, please?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ErnestMatskevich What about this one?

@ErnestMatskevich

Copy link
Copy Markdown
Author

@volodya-lombrozo Please, take a look. All tasks done, all checks passed

@Favixx Favixx left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the work on the inspect entry point. The overall shape (command → Java HTTP server via takes → assembly jar) is reasonable, but I don't think it's mergeable yet — there are a few correctness/build issues plus test-quality gaps. Details inline; most are quick to fix.

Main blockers:

  1. package.json declares mocha three times with conflicting versions — looks like an unresolved merge.
  2. node-fetch is required but is not declared in dependencies (only present transitively).
  3. Server startup relies on a fixed setTimeout(2000), which is racy.
  4. hamcrest is added at compile scope instead of test.

Happy to re-review once these are addressed.

Comment thread package.json
"grunt-mocha-cli": "^7.0.0",
"mocha": "^11.5.0",
"patch-package": "^8.0.0"
"mocha": "10.8.2",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mocha is declared three times in devDependencies here (lines 29, 32, 34: 10.8.2, 11.5.0, ^11.5.0). The last one silently wins, so the first two are dead — this looks like an unresolved merge conflict. Please keep a single mocha entry (^11.5.0) and drop the other two.

Comment thread src/commands/inspect.js
const path = require('path');
const os = require('os');
const readline = require('readline');
const fetch = require('node-fetch');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

node-fetch is required here but it is not listed in dependencies in package.json — it only resolves today because it happens to be hoisted as a transitive dep (^2.6.7 in the lockfile). This will break the moment the transitive tree changes or resolves to node-fetch@3 (ESM-only, where require() throws). Please add node-fetch explicitly to dependencies, pinned to ^2.

Comment thread src/commands/inspect.js
], { cwd: mvnDir, stdio: ['pipe', 'pipe', 'pipe'] });
server.stdout.setEncoding('utf8').on('data', d => console.log(`[SERVER] ${d}`));
server.stderr.setEncoding('utf8').on('data', d => console.error(`[SERVER ERROR] ${d}`));
setTimeout(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Waiting a fixed setTimeout(..., 2000) for the server to come up is a race: on a slow/loaded machine the first request can fire before the port is bound (failure), and on a fast machine you always waste 2s. Better to poll the / endpoint until it answers (or read the server's stdout for a ready marker) and proceed on that signal.

Comment thread src/commands/inspect.js
return new Promise((resolve, reject) => {
const mvnDir = path.join(process.cwd(), 'mvnw');
const jar = path.join(mvnDir, 'target', 'inspect.jar');
const server = spawn('java', [

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No server.on('error', ...) handler. If java isn't on PATH or inspect.jar is missing, spawn emits an error event that is currently unhandled and will crash the process with an opaque stack. Since the surrounding Promise already exposes reject, wire the spawn error to reject(err) with a friendly message (e.g. "run npx grunt first to build inspect.jar").

Comment thread mvnw/pom.xml
</dependency>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest</artifactId>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new hamcrest dependency has no <scope>, so it defaults to compile and ends up on the runtime classpath. Hamcrest is a test-only assertion library — please add <scope>test</scope>.

Comment thread mvnw/pom.xml
<build>
<finalName>eoc</finalName>
<sourceDirectory>${eo.generatedDir}</sourceDirectory>
<sourceDirectory>../inspect/src/main/java</sourceDirectory>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Switching <sourceDirectory> from ${eo.generatedDir} to ../inspect/src/main/java repoints the primary compile root away from the generated EO sources, then re-adds the generated dir via build-helper add-source. This inverts the primary/secondary source roles and is fragile. Please confirm the generated EO sources still compile under every profile (not just the default inspect one), ideally with a comment explaining why the swap is needed.

});
const inspectPromise = inspect({ stdin: input, stdout: out });
input.write('hello\n');
await new Promise(res => setTimeout(res, 5000));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two hard setTimeout(5000) sleeps (10s total) make this test slow and flaky — it's implicitly coupled to the 2000ms startup delay inside inspect.js. Prefer event-driven waiting (resolve when the echoed response arrives) or sinon fake timers, so the test is deterministic and fast.

const stream = require('stream');
const proxyquire = require('proxyquire');
describe('inspect command (mocked)', () => {
it('should send text and receive echoed response, then exit cleanly', async () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only the happy path is covered. There's no test for the catch branch (fetch failure → "Request failed"), for the spawn-error path, or for the Java server itself. At minimum a fetch-failure case would guard the error handling this command depends on.

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

Successfully merging this pull request may close these issues.

4 participants