-
Notifications
You must be signed in to change notification settings - Fork 39
500, adding entry point to inspect command #528
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
base: master
Are you sure you want to change the base?
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? |
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.
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?
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.
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.
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.
mvnw/pom.xml
Outdated
| <build> | ||
| <finalName>eoc</finalName> | ||
| <sourceDirectory>${eo.generatedDir}</sourceDirectory> | ||
| <resources> |
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.
@ErnestMatskevich Why do we need to add this directory to the resources?
mvnw/pom.xml
Outdated
| <finalName>inspect</finalName> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>com.qulice</groupId> |
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.
@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.
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...'); |
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.
@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.
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.
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(); |
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.
@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
Delete unused import Remove redundant variable body Simplified FkRegex with lambda
|
@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 |
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.
@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.
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) | ||
| ); | ||
|
|
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.
@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.
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--
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.
@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.
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); |
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.
@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 |
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'); | ||
|
|
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.
@ErnestMatskevich check this out, please: https://www.yegor256.com/2014/11/03/empty-line-code-smell.html
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 bad, fixed in commit
|
@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.
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?
| } | ||
| console.log('Sending request with body:', input); | ||
| try { | ||
| const response = await fetch('http://localhost:8080/echo', { |
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.
@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.
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?
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.
@ErnestMatskevich What about this one?
Fix puzzle description
|
@volodya-lombrozo Please, take a look. All tasks done, all checks passed |
Renamed branch to #500 for tracking issue
Added test for Inspect.java file