Skip to content

test(#3807): Add disabled test for XMIR bytes with anonymous abstract object#4031

Merged
yegor256 merged 2 commits into
objectionary:masterfrom
volodya-lombrozo:3807_bytes_delta
Mar 22, 2025
Merged

test(#3807): Add disabled test for XMIR bytes with anonymous abstract object#4031
yegor256 merged 2 commits into
objectionary:masterfrom
volodya-lombrozo:3807_bytes_delta

Conversation

@volodya-lombrozo

@volodya-lombrozo volodya-lombrozo commented Mar 21, 2025

Copy link
Copy Markdown
Member

This pull request introduces a new test case in EoSyntaxTest to verify the correct compilation of bytes to XMIR with an anonymous abstract object. The test is currently disabled due to an existing issue with the XMIR format, which is missing an anonymous abstract object for bytes.

Related to #3807

@github-actions

github-actions Bot commented Mar 21, 2025

Copy link
Copy Markdown
Contributor

🚀 Performance Analysis

Test Base Score PR Score Change % Change Unit Mode
benchmarks.XmirBench.xmirToEO 144.689 177.312 32.622 22.55% ms/op Average Time
benchmarks.XmirBench.xmirToPhi 151.999 190.057 38.058 25.04% ms/op Average Time
benchmarks.XmirBench.xmirToSaltyPhi 148.362 126.106 -22.256 -15.00% ms/op Average Time

⚠️ Performance loss: benchmarks.XmirBench.xmirToEO is slower by 32.622 ms/op (22.55%)
⚠️ Performance loss: benchmarks.XmirBench.xmirToPhi is slower by 38.058 ms/op (25.04%)
✅ Performance gain: benchmarks.XmirBench.xmirToSaltyPhi is faster by 22.256 ms/op (15.00%)

@volodya-lombrozo

Copy link
Copy Markdown
Member Author

@maxonfjvipon Is this what we want to achieve in the end?

@maxonfjvipon

Copy link
Copy Markdown
Member

@volodya-lombrozo not really.
For bytes it should be:

<o base="Q.org.eolang.bytes">
  <o>00-...-00</o>
</o>

For numbers and strings:

<o base="Q.org.eolang.string">
  <o base="Q.org.eolang.bytes">
    <o>00-...-00</o>
  </o>
</o>

@maxonfjvipon

maxonfjvipon commented Mar 21, 2025

Copy link
Copy Markdown
Member

@volodya-lombrozo but maybe yes, you're right about

bytes > app
  2A-

It should be:

<o base="Q.org.eolang.bytes" name="app">
  <o base="Q.org.eolang.bytes">
    <o>2A-</o>
  </o>
</o>

In general we should add extra object between Q.org.eolang.bytes and data

Comment thread eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java Outdated
Comment thread eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java Outdated
@volodya-lombrozo

Copy link
Copy Markdown
Member Author

@maxonfjvipon I've moved the test from Java to eo packs. Now they look a bit prettier. Also I've added tests for number and string. What do you think?

@maxonfjvipon maxonfjvipon 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.

@volodya-lombrozo LGTM! Thanks

@volodya-lombrozo

Copy link
Copy Markdown
Member Author

@yegor256 Could you merge that changes, please?

@yegor256 yegor256 enabled auto-merge March 22, 2025 06:03
@yegor256

Copy link
Copy Markdown
Member

@volodya-lombrozo once we "resolve" all conversations above, GitHub will merge this one automatically

@yegor256 yegor256 added this pull request to the merge queue Mar 22, 2025
Merged via the queue into objectionary:master with commit 970d03a Mar 22, 2025
@volodya-lombrozo

Copy link
Copy Markdown
Member Author

@yegor256 It's a bit risky to merge after all the conversations are resolved because I can resolve them myself.

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.

3 participants