I have an idea for a (probably non-trivial) improvement to the test suite. While porting :erlang.make_tuple/2, I made a mistake and wrote a test checking that making a tuple with size 0 raises an error. This is not true, an empty tuple is returned instead, but the test passed, since my implementation raised an error.
Would it be possible to compare the result of the JS implementation with the result of the actual Erlang function? I assume this would require implementing at least one of term_to_binary and binary_to_term. Does this seem too difficult to be worth the effort?
Thanks for the feedback! Have you seen the “Server-Side Consistency Tests” section in the Contributing Guide? The idea is that you implement matching tests in Elixir (in test/elixir/hologram/ex_js_consistency/erlang/) that mirror your JavaScript tests. These verify your JS implementation behaves identically to the actual OTP implementation. And if OTP behavior changes in the future, it’ll be caught.
Does this resolve the issue for your :erlang.make_tuple/2 case, or should we make this clearer in the docs?
Also, could you elaborate on your term_to_binary idea? I’m curious what you have in mind.
Ah, I misunderstood the point of the Elixir tests. I thought they were run using the JS implementation as well. The actual problem seems to be elsewhere, probably in `assert_error` reporting success even when there is no error. I’m investigating further.
As for the idea regarding comparing the two implementations, I think there would need to be a common serialization format, term_to_binary being the obvious choice. Then, a piece of code could be run both on the BEAM and in JS and the outputs could be compared. This should remove the need for duplicating tests.
EDIT: Yes, as far as I can tell, assert_error does not actually check that an error was raised. That explains my confusion when the empty tuple test passed even though there was no error.
Erlang consistency tests will eventually be automatically transpiled and run on the client. To enable that, we need to port some Erlang functions first (including some from phase 2). For now, matching tests are the simplest and most maintainable way to verify consistency IMO. The same applies to Elixir stdlib tests - they will be transpiled as well eventually to automatically verify consistency.
Could you share code snippets that reproduce the problem? I looked at your PR (https://github.com/bartblast/hologram/pull/363/files) and all CI checks pass, including tests using assert_error. If you have a specific case where it passes when it shouldn’t, please share the test code and what you expected vs. what happened.
I fixed the tests in the second commit, but the first commit contains the test that should not be passing.
test "raises ArgumentError when arity is not positive" do
assert_error ArgumentError,
build_argument_error_msg(1, "out of range"),
{:erlang, :make_tuple, [0, :a]}
end
There is no error when applying the function, it simply returns the empty tuple. Therefore, I would expect the test to fail, since it should assert that an error was raised.
Looking at the definition of assert_error, I don’t immediately see anything asserting that there was actually an error.
@bartblast I’m curious on your thoughts of what the return value of re.version should be, considering there is not a single version tied to JavaScript Regex capabilities?
❯ iex
Erlang/OTP 27 [erts-15.2.2] [source] [64-bit] [smp:16:16] [ds:16:16:10] [async-threads:1] [jit]
Interactive Elixir (1.18.2) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> h :re.version/0
@spec version() :: binary()
since: OTP 20.0
The return of this function is a string with the PCRE version of the system
that was used in the Erlang/OTP compilation.
❯ pcre-config --version
8.45
I simply return an empty bitstring for now, bit this test will always fail.
describe "version/0" do
test "empty string" do
assert :re.version() == ""
end
end
Since JavaScript regexes aren’t compatible with the PCRE standard (which Elixir uses), Hologram will eventually transpile them to maintain compatibility.
For now, please hardcode the JavaScript implementation to return "8.44 2020-02-12". In both your JavaScript tests and Elixir consistency tests, use this regex pattern to verify the result format: ~r/^\d+\.\d+\s+\d{4}-\d{2}-\d{2}$/ (this should work in both Elixir and JS)
This way both test suites validate the version string format rather than an exact value, which keeps things consistent.
Eventually, :re.version/0 will fetch the PCRE version from the Hologram client-runtime, which will receive it from the server in the initial request. Initially we’ll be targeting a specific PCRE version to ensure consistency across the client-side regex behavior.
I haven’t had a chance to look at the :sets module PRs yet, so I can’t give informed advice. Maybe @Lucassifoni can weigh in, since he worked on the module - curious to hear your thoughts, Lucas! From what I understand, they are just maps in the newest (version 2) implementation.
Not all :sets functions need that shimming though. But ultimately returning a map with empty lists as values is the correct thing to do, and we should emit errors (non replicable in elixir consistency tests) if someone really wants to use :sets v1 (the tuple buckets data structure).
To expand on that, the main sets handling module in Elixir, MapSet, is dubbed MapSet because it used an elixir implementation with maps having lists as values, before switching to :sets when the erlang implementation adopted this simpler data representation, I think as of Elixir 1.15.
Luckily we do not need to support OTP < 24 nor Elixir 1.15 in Hologram
I think I need to wait on @Lucassifoni‘s work to get merged before submitting a PR for :sets.to_list/1. I’ve encountered a certain wrinkle with consistency tests, and I’m not sure what approach to take. Specifically, Erlang sets may contain any data type for its keys, and treats them uniquely.
Yeah that’s annoying. I need to think about how to circumvent it. Hologram-JS side we should use Type.encodeMapKey. But we need to ensure we can get values out of that, while not using another data representation as valid elixir code could depend on the internal structure of sets.
I have a bit of time this week and would be glad to collaborate on this module
Read the full blog post: https://hologram.page/blog/elixir-to-javascript-porting-initiative
