500, adding entry point to inspect command#528
Conversation
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
|
I added simple test for Also I would like to clarify how to build There is 1 falling check in eslint workflow Is it on my side? If it is how can I fix that? |
|
@ErnestMatskevich I fixed eslint, please merge master into your branch |
|
@ErnestMatskevich show what you are trying to do and show the error (in CI logs). We'll be able to provide help. |
|
@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
|
@yegor256 I made profile in I have some questions:
|
|
@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. |
|
@ErnestMatskevich it's better to ask questions as separate tickets, see: https://www.yegor256.com/2025/05/25/bug-driven-development.html |
|
@volodya-lombrozo can you please check this design? |
volodya-lombrozo
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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?
|
@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. |
Yes, it's what debuggers allow me to do - to "inspect" runtime. |
|
@volodya-lombrozo Thank you for your review feedback. I've made the following improvements:
For the current changes in pom.xml added Maven profile section to build self-contained inspect command can be run with help node src/eoc.js inspect or it is possible just build Could you please review this profile configuration approach. |
| server.start(); | ||
| Thread.sleep(2000L); | ||
| server.interrupt(); | ||
| server.join(1000L); |
There was a problem hiding this comment.
@ErnestMatskevich Could you add clear assert statements, please? Here is a good paragraph how you can do it.
| <build> | ||
| <finalName>eoc</finalName> | ||
| <sourceDirectory>${eo.generatedDir}</sourceDirectory> | ||
| <resources> |
There was a problem hiding this comment.
@ErnestMatskevich Why do we need to add this directory to the resources?
| <finalName>inspect</finalName> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>com.qulice</groupId> |
There was a problem hiding this comment.
@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.
| module.exports = function(opts) { | ||
| return new Promise((resolve, reject) => { | ||
| const mvnDir = path.join(process.cwd(), 'mvnw'); | ||
| console.info('Building EO inspect server with Maven...'); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@volodya-lombrozo @ErnestMatskevich indeed, it's better to build it once, when we package eoc. Then, simply run it.
| 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(); |
There was a problem hiding this comment.
@ErnestMatskevich Can we simplify these variable names somehow? A Compound Name Is a Code Smell
Added asserts in InspectSmokeTest.java and changed Inspect.java to handle interrupt
Delete qulice plugin and redundant resources section in pom.xml
|
@volodya-lombrozo I fixed feedback items from your review. I splitted them into 2 commits:
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 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
|
@yegor256 @volodya-lombrozo I changed I wrote descriptor I deleted Maven part from inspect.js since There are problems in workflow with build and itests:
Could't find object 'Φ.org.eolang.io' Could you please take a look for my commit and help to fix build |
volodya-lombrozo
left a comment
There was a problem hiding this comment.
@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() { |
There was a problem hiding this comment.
@ErnestMatskevich Great thanks for the explanation. Now it's clear.
| server.isAlive(), | ||
| is(true) | ||
| ); | ||
|
|
There was a problem hiding this comment.
@ErnestMatskevich Can we remove empty lines, please? It's a code smell.
| } | ||
| assertThat( | ||
| "Server thread should be running after start", | ||
| server.isAlive(), |
There was a problem hiding this comment.
@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--
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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); |
There was a problem hiding this comment.
@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
|
@volodya-lombrozo Thank you for your detail reviewes! I committed last changes. In InspectTest.java:
I opened issue for failing workflow Could you please check last commit and help to understand reason of failing. |
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
Summary for commitUpdated 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: There are such files in 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
Fixed all errors
@yegor256 Please, take a look |
| return new Promise((resolve, reject) => { | ||
| const mvnDir = path.join(process.cwd(), 'mvnw'); | ||
| const jar = path.join(mvnDir, 'target', 'inspect.jar'); | ||
|
|
There was a problem hiding this comment.
@ErnestMatskevich check this out, please: https://www.yegor256.com/2014/11/03/empty-line-code-smell.html
|
@ErnestMatskevich looks good! I'm not sure though, it's a good idea to keep source files in the |
|
@volodya-lombrozo please, take a look one more time |
@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 ( |
|
@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 |
Extract source code files from mvnw and place them in inspect folder
Yes, I do not work with wrapper directly, i just build |
|
@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.
|
| * | ||
| * @since 0.29.0 | ||
| */ | ||
| final class InspectTest { |
There was a problem hiding this comment.
@ErnestMatskevich It seems, that this test checks nothing. Moreover, it's too error prone. Can we just remove it?
| } | ||
| console.log('Sending request with body:', input); | ||
| try { | ||
| const response = await fetch('http://localhost:8080/echo', { |
There was a problem hiding this comment.
@ErnestMatskevich What if I already have another application running on 8080 port? Please, don't fix it here. Just leave a puzzle
| </build> | ||
| <profiles> | ||
| <profile> | ||
| <id>inspect</id> |
There was a problem hiding this comment.
@ErnestMatskevich I can't find the place in the code where you use this Maven profile. Can you give me a link, please?
Fix puzzle description
|
@volodya-lombrozo Please, take a look. All tasks done, all checks passed |
Favixx
left a comment
There was a problem hiding this comment.
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:
package.jsondeclaresmochathree times with conflicting versions — looks like an unresolved merge.node-fetchisrequired but is not declared independencies(only present transitively).- Server startup relies on a fixed
setTimeout(2000), which is racy. hamcrestis added at compile scope instead of test.
Happy to re-review once these are addressed.
| "grunt-mocha-cli": "^7.0.0", | ||
| "mocha": "^11.5.0", | ||
| "patch-package": "^8.0.0" | ||
| "mocha": "10.8.2", |
There was a problem hiding this comment.
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.
| const path = require('path'); | ||
| const os = require('os'); | ||
| const readline = require('readline'); | ||
| const fetch = require('node-fetch'); |
There was a problem hiding this comment.
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.
| ], { 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(() => { |
There was a problem hiding this comment.
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.
| return new Promise((resolve, reject) => { | ||
| const mvnDir = path.join(process.cwd(), 'mvnw'); | ||
| const jar = path.join(mvnDir, 'target', 'inspect.jar'); | ||
| const server = spawn('java', [ |
There was a problem hiding this comment.
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").
| </dependency> | ||
| <dependency> | ||
| <groupId>org.hamcrest</groupId> | ||
| <artifactId>hamcrest</artifactId> |
There was a problem hiding this comment.
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>.
| <build> | ||
| <finalName>eoc</finalName> | ||
| <sourceDirectory>${eo.generatedDir}</sourceDirectory> | ||
| <sourceDirectory>../inspect/src/main/java</sourceDirectory> |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
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.
Renamed branch to #500 for tracking issue
Added test for Inspect.java file