Julia #126

rystidia · 2024-04-29T09:41:18Z

Add tests for compiled tests, running scripts, and KST translator support for Julia

This PR addresses the issue kaitai-io/kaitai_struct#1109

GreyCat · 2024-04-29T16:03:05Z

Thanks for your contribution, @rystidia! Can I also ask to take one step further and Julia testing runs in Docker? This way:

we can do stuff like ./docker-ci -t julia -i some_version to run these tests locally without installing Julia manually,
and, probably more importantly, it streamlines CI integration a lot, as we can just reuse exactly the image and results you've created like that.

It should be relatively straightforward to create a Julia image in kaitai_struct_docker_images, following the pattern of existing images.

rystidia · 2024-04-29T20:08:50Z

I created this PR (which was merged). It worked for me when running like ./docker-ci -t julia -i 1.9. Should I somehow change it?

GreyCat · 2024-04-29T22:23:50Z

Nope, sorry, please disregard my previous comment — I've just overlooked that you've already created it. Now it's merged and this enables to test tests locally.

generalmimon · 2024-06-10T08:09:02Z

run-julia

@@ -0,0 +1,10 @@
+#!/bin/bash


This is the only run-* script with the #!/bin/bash shebang - all the other run-* scripts use mostly #!/bin/sh -ef. I don't see any reason to use #!/bin/bash here, given that the script ~~apparently doesn't~~ shouldn't have to use any bashisms. Using #!/bin/sh -ef instead will make it more portable, for example it will then work on Alpine Linux that doesn't have bash by default.

Edit: oh, source is actually a bashism, as ShellCheck points out:

Line 4: source ./config ^-- SC3046 (warning): In POSIX sh, 'source' in place of '.' is undefined.

Well, that's another good reason to use . instead of source, as I suggest below.

generalmimon · 2024-06-10T08:09:44Z

run-julia

+
+

Double newline here:

Suggested change

generalmimon · 2024-06-10T08:10:09Z

run-julia

+#!/bin/bash
+
+
+source ./config


We use

Suggested change

source ./config

. ./config

everywhere.

generalmimon · 2024-06-10T08:30:31Z

run-julia

+# Define the command to run the Julia script with redirected output
+command="JULIA_LOAD_PATH=\"$JULIA_LOAD_PATH:$JULIA_RUNTIME_DIR:compiled/julia:spec/julia/extra\" julia \"spec/julia/runtests.jl\""
+
+# Run the command
+eval $command


I don't see any reason to use eval here. It makes everything ugly by having to escape the double quotes as \", and given that the $command in eval $command is unquoted, it is subject to all the globbing and word splitting as described in SC2086, which in this case means that JULIA_LOAD_PATH and JULIA_RUNTIME_DIR must not contain any spaces or glob characters, otherwise the script might break.

Why not just:

Suggested change

# Define the command to run the Julia script with redirected output

command="JULIA_LOAD_PATH=\"$JULIA_LOAD_PATH:$JULIA_RUNTIME_DIR:compiled/julia:spec/julia/extra\" julia \"spec/julia/runtests.jl\""

# Run the command

eval $command

JULIA_LOAD_PATH="$JULIA_LOAD_PATH:$JULIA_RUNTIME_DIR:compiled/julia:spec/julia/extra" julia spec/julia/runtests.jl

Also, I don't see the value of trivial comments such as Define the command to run the Julia script with redirected output (and I don't think this one actually makes sense - what exactly does "with redirected output" refer to? - I don't see any |, > or >> after the command, so it doesn't look like the output is redirected anywhere).

generalmimon · 2024-06-21T16:58:00Z

aggregate/junit_xml_parser.rb

@@ -71,6 +74,15 @@ def each_test

          tr = TestResult.new(name, :skipped, 0, nil)
          yield tr
+        elsif ts.attribute('errors') && ts.attribute('errors').value.to_f != 0 && ts.attribute('name').value =~ /^\/(.*?) test$/


Parsing the errors attribute as a float (.to_f) doesn't make sense. It should be parsed as an integer, preferably using the strictest way possible, which would be Integer(..., 10) (see https://stackoverflow.com/a/49282/12940655).

Alternatively, we might not need to parse it at all, but instead can compare it directly to the string representation like ts.attribute('errors').value != '0' (this is even stricter, but all we really need, since there shouldn't be any other representations of zero anyway).

generalmimon · 2024-06-21T17:29:34Z

spec/julia/runtests.jl

+function run_tests()
+    #nested testset is needed to continue even after some test errors
+    @testset "" begin
+        for f in filter(s -> contains(s, r".jl$") && !contains(s, "runtests.jl"), readdir(@__DIR__; join=true))


This:

contains(s, r".jl$")

... is an unnecessary use of a regex, which actually comes with a subtle bug - the dot . doesn't mean "literal dot" in this context, but "arbitrary character":

julia> print(contains("hjl", r".jl$")) true

This demonstrates that it is generally best to use the "least powerful" tool for the job - in this case endswith:

julia> print(endswith("hjl", ".jl")) false julia> print(endswith("h.jl", ".jl")) true

Simpler tools tend to have less surprises.

To me, this is sloppy:

!contains(s, "runtests.jl")

This tests whether the string "runtests.jl" is contained anywhere in the (absolute) file path, which is not really what we want (there's no reason to match helloruntests.jl or directories helloruntests.jl/..., etc.). What we actually want is to take the basename of that path and test it for equality with the string "runtests.jl".

generalmimon · 2024-06-21T17:38:23Z

translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/JuliaSG.scala

+  override def header(): Unit = {
+    importList.add(s"import ${className}Module")
+    out.puts
+    out.puts(s"@testset ${'"'}$className test${'"'} begin")


To me, this way of inserting the literal double quote is unnecessarily complicated - this is how we usually do it:

Suggested change

out.puts(s"@testset ${'"'}$className test${'"'} begin")

out.puts(s"""@testset "$className test" begin""")

See for example:

kaitai_struct_tests/translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/LuaSG.scala

Line 33 in 44ffc60

out.puts(s"""local r = $className:from_file("src/${spec.data}")""")

generalmimon · 2024-06-21T17:45:10Z

translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/JuliaSG.scala

+  override def noAsserts() =
+    out.puts("")


Any particular reason to override noAsserts just to insert a blank line? All other languages except Python don't override it and just use the default (empty) implementation:

kaitai_struct_tests/translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/BaseGenerator.scala

Line 32 in 44ffc60

def noAsserts(): Unit = {}

Python is the only language which overrides it, but that actually serves a purpose (Python syntactically requires at least one statement in each block, and pass is idiomatically used if there's nothing to do):

kaitai_struct_tests/translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/PythonSG.scala

Lines 77 to 78 in 44ffc60

override def noAsserts() =

out.puts("pass")

generalmimon · 2024-06-21T17:50:32Z

translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/JuliaSG.scala

+  override def simpleEquality(check: TestEquals): Unit = {
+    val actStr = translateAct(check.actual)
+    val expStr = translateExp(check.expected)
+    out.puts(s"@test $actStr == $expStr")
+  }
+
+  override def floatEquality(check: TestEquals): Unit = {
+    val actStr = translateAct(check.actual)
+    val expStr = translator.translate(check.expected)


This inconsistency doesn't look intentional - either both should use translateExp, or both should use translator.translate.

generalmimon · 2024-06-21T18:07:57Z

translator/src/main/scala/io/kaitai/struct/testtranslator/specgenerators/JuliaSG.scala

+  def translateExp(x: Ast.expr) =
+    translator.translate(x).replace("this._root", className)


I don't like seeing this pattern at all since:

it uses a textual replacement... (which is inherently wrong and should be avoided as much as possible - textual replacement can be only ever appropriate in free-form text, not in anything that has certain syntax and semantics that must be respected, like the code expression of some programming language. The existing .replace("this." + Main.INIT_OBJ_NAME, "r") is also fundamentally flawed, but at least it is very unlikely to cause any actual problems, because val INIT_OBJ_NAME = "q1w2e3" is sufficiently "random" that it probably won't clash with anything unintended.)

with something that doesn't even make sense at first sight (in general you can hardly "replace" _root, which is the reference to the root instance of the top-level class, with the class name itself. That is like apples and oranges).

This shouldn't be necessary, and I'm afraid it indicates some issues with the compiler code generation...

rystidia · 2024-06-22T15:05:02Z

@generalmimon Thanks for your detailed review. As a reaction to your suggested changes, I opened !127

Dias Rystin added 8 commits April 26, 2024 14:13

First version of Julia support for translator

f8fa21f

add the support of julia to KST translator

af1171d

generate tests for julia

f4a9743

add julia support for aggregator scripts

436b497

add scripts to run julia tests

7539986

add kst_adoption to julia ci running script

aa86b0d

fix ci-julia

4c592cb

use existing case for julia

6879a2f

GreyCat merged commit 6879a2f into kaitai-io:master Jun 10, 2024

generalmimon reviewed Jun 21, 2024

View reviewed changes

rystidia mentioned this pull request Jun 22, 2024

Julia tests #127

Open

generalmimon mentioned this pull request Aug 20, 2024

translator build broken -- not found: value JuliaCompiler kaitai-io/kaitai_struct#1127

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Julia #126

Julia #126

rystidia commented Apr 29, 2024

GreyCat commented Apr 29, 2024

rystidia commented Apr 29, 2024

GreyCat commented Apr 29, 2024

generalmimon Jun 10, 2024

generalmimon Jun 10, 2024

generalmimon Jun 10, 2024

generalmimon Jun 10, 2024

generalmimon Jun 21, 2024 •

edited

Loading

generalmimon Jun 21, 2024

generalmimon Jun 21, 2024

generalmimon Jun 21, 2024

generalmimon Jun 21, 2024

generalmimon Jun 21, 2024 •

edited

Loading

rystidia commented Jun 22, 2024

	out.puts(s"@testset ${'"'}$className test${'"'} begin")
	out.puts(s"""@testset "$className test" begin""")

		def translateExp(x: Ast.expr) =
		translator.translate(x).replace("this._root", className)

Julia #126

Julia #126

Conversation

rystidia commented Apr 29, 2024

GreyCat commented Apr 29, 2024

rystidia commented Apr 29, 2024

GreyCat commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

generalmimon Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

generalmimon Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

rystidia commented Jun 22, 2024

generalmimon Jun 21, 2024 •

edited

Loading

generalmimon Jun 21, 2024 •

edited

Loading