Make test suite work in OTP 25 #1402

fabjan · 2022-11-08T07:41:53Z

Problem

The hover and completion provider test suites have some test cases for showing docs.

Some tests are testing the Erlang LS internal fallback docs provided if EEP-48 docs can not be extracted. These test cases don't work with newer OTP versions (because their expected values depend on the EEP-48 docs not being available).

Some of the tests are testing edoc extraction itself, and have conditional expectations depending on if EEP-48 docs are available or not. These tests assume that OTP >= 24 will always have EEP-48 docs for file:write if code:get_docs(file) is not empty, but this is not the case (see e.g. #1402 (comment)). The test suites and the code in els_docs are not in agreement about when the docs are available.

Proposed Solution

Mock the els_docs eep48 calls to say eep48 is unavailable for the tests that test that fallback
Skip the OTP module specific tests if OTP was not built with doc chunks (as far as I can tell, those test cases don't test any Erlang LS code that other test cases don't cover)

If the doc chunks are expected to be there in OTP for anyone running the test suite, perhaps we can document how to resolve that (as it is now, I don't know how to make the test suite pass locally).

Bonus Content

Add OTP 25 to CI.
Drop OTP 22 from CI.

apps/els_lsp/test/els_hover_SUITE.erl

fabjan · 2022-11-13T16:34:37Z

apps/els_lsp/test/els_completion_SUITE.erl

+    ?assertMatch(
+        #{
+            data := #{module := <<"file">>, function := <<"write">>, arity := 2},
+            documentation := #{kind := <<"markdown">>, value := _}
+        },
+        Result
+    ),
+    #{documentation := #{value := ActualValue}} = Result,


This refactor was made to make case the last part of the expression, so it can return {skip, _}. A similar hoist was applied to all cases where I added skip.

(The data := #{module := <<"file">>, function := <<"write">>, arity := 2} check is new though.)

Ah, this is no longer useful after the latest few commits in this branch.

fabjan · 2022-11-13T16:43:46Z

apps/els_lsp/test/els_completion_SUITE.erl

+            %% If this should fail the test we should document requirements for
+            %% the development environment so that OTP has the EEP-48 docs.
+            {skip, "This OTP build does not have EEP-48 docs for the 'file' module"};


Maybe you want this to fail instead of skipping. If so, I think the docs should be updated to ensure developers have the right stuff locally to run the test suite.

I have OTP 24 and 25 installed via asdf and code:get_doc(file) yields a map with docs, but no functions have docs:

1> {ok, {docs_v1, 0, erlang, <<"application/erlang+html">>, none, _, ModDocs}} = code:get_doc(file). 2> [{F, A, Doc} || {{function, F, A}, _, _, Doc, _} <- ModDocs]. [{native_name_encoding,0,none}, {format_error,1,none}, {pid2name,1,none}, {get_cwd,0,none}, {get_cwd,1,none}, {set_cwd,1,none}, {delete,1,none}, {delete,2,none}, {rename,2,none}, {...}|...] 3> [{F, A, Doc} || {{function, F, A}, _, _, Doc, _} <- ModDocs, Doc =/= none]. []

fabjan · 2022-11-13T16:47:01Z

apps/els_lsp/test/els_hover_SUITE.erl

+            lists:any(
+                fun
+                    ({_, _, _, #{}, _}) -> true;
+                    ({_, _, _, _, _}) -> false
+                end,
+                Docs
+            );


This and has_eep48/1 in the other suite were only updated to know when the cases can be skipped. If skipping is not wanted this can go away (or be used to improve test debugging).

fabjan · 2023-02-09T19:18:39Z

Feel free to close this PR if there is too much else going on. I prefer to not be the one closing it since it is not targeting my repository.

Maybe main has moved too much anyway by now (didn't look) for this to make sense.

robertoaloi · 2023-02-11T08:46:26Z

Hi @fabjan, sorry for not being responsive to Pull Requests in the past couple of months. The contribution looks good to me, but the CI jobs seem to have expired and I cannot find a way to restart them. If you could rebase/force-push this branch we can merge this one, assuming CI passes.

fabjan · 2023-02-11T14:58:42Z

No worries at all. Sure I'll give it a try!

robertoaloi · 2023-02-15T08:01:00Z

@fabjan It looks like a test suite is still failing on this one.

fabjan · 2023-02-15T18:42:54Z

Cheers, Time to look at this one now after the rebases and other PR was merged 👍

The test suites work locally for me with OTP 24 and 25.

fabjan · 2023-02-15T19:33:13Z

Alright let's see now how it plays. I got all green checks running locally on OTP 23 so my "fix" is a guess, but I think it is sound: de4f89d

fabjan · 2023-02-16T21:44:50Z

Can't get OTP 22 to work on my M1 Mac, so my attempts at making the test suites work for all releases are being made a little bit in the dark.

fabjan · 2023-02-16T21:55:50Z

@robertoaloi An attempt was made, but I think I have to give up on this PR.

I don't know enough about how the test suite or the docs rendering feature was built to make a call on how to fix the test suite.

I can't install OTP 22 locally to test.
-if(?OTP_RELEASE >= 23). is used to conditionally compile/include/export some of the code used.
-ifdef(NATIVE_FORMAT). is used to conditionally compile some other code.
list_to_integer(erlang:system_info(otp_release)) >= 24. is used to select some test code.
The test code in addition to the above depends on exactly how the local OTP you use was compiled (does the stdlib have doc chunks).

This PR was a much bigger task than I saw before me. It seems to make it work the test code needs a bigger refactor.

robertoaloi · 2023-02-17T07:39:47Z

Hi @fabjan I think it's ok to drop support for OTP 22. They just announced the RC1 for OTP 26. So, feel free to remove it from the pipeline.

fabjan · 2023-02-17T19:27:41Z

I bumped the Windows CI job to OTP 23 as well (it was at 22). Interestingly, it was green last time even though it seems like it tested nothing: https://github.com/erlang-ls/erlang_ls/actions/runs/4198452678/jobs/7282095992

Maybe now with OTP 23 it will run the tests and go red..

fabjan · 2023-02-18T09:19:49Z

apps/els_lsp/test/els_hover_SUITE.erl

-        case has_eep48_edoc() of
-            true ->
+        case list_to_integer(erlang:system_info(otp_release)) of
+            25 ->
                <<
-                    "## j/1\n\n---\n\n```erlang\n\n  j(_) \n\n```\n\n"
-                    "```erlang\n-spec j(doesnt:exist()) -> ok.\n```"


I think there is a bug in els_eep48_docs:render when running in OTP versions under 25, and that is why this current test code looks a bit weird (it checks if has_eep48_edoc() is true or false but uses the same test data regardless)

On OTP 24 locally on my machine, els_eep48_docs:render returns {error, function_missing} for this j function that is hovered. On OTP 25 it returns some kind of doc chunks instead even of the function is not documented. Maybe OTP started automatically adding doc chunks for specs?

Anywho, I didn't want to make changes to the production code in this PR adding OTP 25 to CI, so not diving into if els_eep48_docs:render should be improved (especially since it seems to me that it's under OTP 24 it is behaving badly).

fabjan · 2023-02-18T10:02:14Z

Converted to draft until the useless diffs are cleaned up.

But CI is all green now!

It was not my intention to make it a minute, I just wanted a significant bump up from one second. 640 centiseconds ought to be enough for anyone.

robertoaloi · 2023-02-20T08:43:02Z

apps/els_lsp/test/els_hover_SUITE.erl

@@ -78,12 +78,21 @@ init_per_testcase(TestCase, Config) ->

 -spec end_per_testcase(atom(), config()) -> ok.
 end_per_testcase(TestCase, Config) ->
+    lists:foreach(


The fact that meck registers a module with the _meck suffix is an implementation detail that we should not rely on. We should only unload els_docs for those testcases where the module is mocked.

The correct way to do "unmock if mocked" is to use the meck API instead. meck:mocked returns a list of mocked modules.

Ah nice to know 👍

robertoaloi · 2023-02-21T08:21:38Z

Thanks!

* Fix EEP-48 case for the nonexisting_type test * Make hover docs fallback tests avoid EEP-48 docs * Fix hover suite formatting * Make nonexisting_type test work in OTP 24 and 25 * Improve mocking in eep48 fallback tests * Skip OTP EEP-48 tests if OTP is built without docs * Add OTP 25 to CI The test suites work locally for me with OTP 24 and 25. * Fix eep48 check in remote_call_otp test * Make tests work if els_eep48_docs is undefined * Remove OTP 22 from CI * fmt * Fix eep48 check in resolve_application_remote_otp * Make has_eep48 test helper more readable * Fix eep48 in resolve_type_application_remote_otp * Silence els_config warnings in PropEr suite * Increase halt_called wait timeout in PropEr suite * Cleanup WIP comment in nonexisting_type test * Comment the version check in nonexisting_type test * Clean up useless leftover diffs * Fix halt_called timeout in PropEr suite It was not my intention to make it a minute, I just wanted a significant bump up from one second. 640 centiseconds ought to be enough for anyone. * Only unload els_docs mock after cases that used it * fmt

fabjan commented Nov 13, 2022

View reviewed changes

apps/els_lsp/test/els_hover_SUITE.erl Outdated Show resolved Hide resolved

fabjan commented Nov 13, 2022

View reviewed changes

fabjan changed the title ~~Make hover suite work in OTP 25~~ Make test suite work in OTP 25 Nov 17, 2022

superherointj mentioned this pull request Jan 16, 2023

erlang: set version to latest NixOS/nixpkgs#211081

Merged

fabjan force-pushed the fix-hover-suite branch from b29f6ab to d45b3c9 Compare February 11, 2023 19:10

fabjan added 8 commits February 15, 2023 20:31

Fix EEP-48 case for the nonexisting_type test

69b6047

Make hover docs fallback tests avoid EEP-48 docs

9c5d5d1

Fix hover suite formatting

c7463bd

Make nonexisting_type test work in OTP 24 and 25

9c4cfb0

Improve mocking in eep48 fallback tests

9b12c7c

Skip OTP EEP-48 tests if OTP is built without docs

746e727

Add OTP 25 to CI

2887d3d

The test suites work locally for me with OTP 24 and 25.

Fix eep48 check in remote_call_otp test

de4f89d

fabjan force-pushed the fix-hover-suite branch from d45b3c9 to de4f89d Compare February 15, 2023 19:31

Make tests work if els_eep48_docs is undefined

b1bc61c

Remove OTP 22 from CI

0e15306

fabjan added 2 commits February 17, 2023 20:33

fmt

ab98ee5

Fix eep48 check in resolve_application_remote_otp

09f09b5

fabjan added 6 commits February 17, 2023 21:00

Make has_eep48 test helper more readable

3dcd3e1

Fix eep48 in resolve_type_application_remote_otp

ae4eda0

Silence els_config warnings in PropEr suite

222c07e

Increase halt_called wait timeout in PropEr suite

f1e265c

Cleanup WIP comment in nonexisting_type test

187acdb

Comment the version check in nonexisting_type test

3db8e76

fabjan commented Feb 18, 2023

View reviewed changes

fabjan marked this pull request as draft February 18, 2023 10:01

fabjan added 2 commits February 18, 2023 11:48

Clean up useless leftover diffs

13db044

Fix halt_called timeout in PropEr suite

2769499

It was not my intention to make it a minute, I just wanted a significant bump up from one second. 640 centiseconds ought to be enough for anyone.

fabjan marked this pull request as ready for review February 18, 2023 10:56

robertoaloi reviewed Feb 20, 2023

View reviewed changes

fabjan added 2 commits February 20, 2023 21:22

Only unload els_docs mock after cases that used it

ca1aa6c

fmt

27601e3

robertoaloi merged commit cfcf610 into erlang-ls:main Feb 21, 2023

fabjan mentioned this pull request Feb 21, 2023

Add OTP 25 to CI #1216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make test suite work in OTP 25 #1402

Make test suite work in OTP 25 #1402

fabjan commented Nov 8, 2022 •

edited

Loading

fabjan Nov 13, 2022 •

edited

Loading

fabjan Feb 18, 2023

fabjan Nov 13, 2022 •

edited

Loading

fabjan Nov 13, 2022

fabjan commented Feb 9, 2023

robertoaloi commented Feb 11, 2023

fabjan commented Feb 11, 2023

robertoaloi commented Feb 15, 2023

fabjan commented Feb 15, 2023

fabjan commented Feb 15, 2023

fabjan commented Feb 16, 2023

fabjan commented Feb 16, 2023 •

edited

Loading

robertoaloi commented Feb 17, 2023

fabjan commented Feb 17, 2023

fabjan Feb 18, 2023

fabjan commented Feb 18, 2023

robertoaloi Feb 20, 2023

TheGeorge Feb 20, 2023

fabjan Feb 20, 2023

robertoaloi commented Feb 21, 2023

Make test suite work in OTP 25 #1402

Make test suite work in OTP 25 #1402

Conversation

fabjan commented Nov 8, 2022 • edited Loading

Problem

Proposed Solution

Bonus Content

fabjan Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

fabjan Feb 18, 2023

Choose a reason for hiding this comment

fabjan Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

fabjan Nov 13, 2022

Choose a reason for hiding this comment

fabjan commented Feb 9, 2023

robertoaloi commented Feb 11, 2023

fabjan commented Feb 11, 2023

robertoaloi commented Feb 15, 2023

fabjan commented Feb 15, 2023

fabjan commented Feb 15, 2023

fabjan commented Feb 16, 2023

fabjan commented Feb 16, 2023 • edited Loading

robertoaloi commented Feb 17, 2023

fabjan commented Feb 17, 2023

fabjan Feb 18, 2023

Choose a reason for hiding this comment

fabjan commented Feb 18, 2023

robertoaloi Feb 20, 2023

Choose a reason for hiding this comment

TheGeorge Feb 20, 2023

Choose a reason for hiding this comment

fabjan Feb 20, 2023

Choose a reason for hiding this comment

robertoaloi commented Feb 21, 2023

fabjan commented Nov 8, 2022 •

edited

Loading

fabjan Nov 13, 2022 •

edited

Loading

fabjan Nov 13, 2022 •

edited

Loading

fabjan commented Feb 16, 2023 •

edited

Loading