Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make test suite work in OTP 25 #1402

Merged
merged 22 commits into from
Feb 21, 2023
Merged

Make test suite work in OTP 25 #1402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
69b6047
Fix EEP-48 case for the nonexisting_type test
fabjan Nov 5, 2022
9c5d5d1
Make hover docs fallback tests avoid EEP-48 docs
fabjan Nov 5, 2022
c7463bd
Fix hover suite formatting
fabjan Nov 6, 2022
9c4cfb0
Make nonexisting_type test work in OTP 24 and 25
fabjan Nov 13, 2022
9b12c7c
Improve mocking in eep48 fallback tests
fabjan Nov 13, 2022
746e727
Skip OTP EEP-48 tests if OTP is built without docs
fabjan Nov 13, 2022
2887d3d
Add OTP 25 to CI
fabjan Nov 13, 2022
de4f89d
Fix eep48 check in remote_call_otp test
fabjan Feb 15, 2023
b1bc61c
Make tests work if els_eep48_docs is undefined
fabjan Feb 16, 2023
0e15306
Remove OTP 22 from CI
fabjan Feb 17, 2023
ab98ee5
fmt
fabjan Feb 17, 2023
09f09b5
Fix eep48 check in resolve_application_remote_otp
fabjan Feb 17, 2023
3dcd3e1
Make has_eep48 test helper more readable
fabjan Feb 17, 2023
ae4eda0
Fix eep48 in resolve_type_application_remote_otp
fabjan Feb 17, 2023
222c07e
Silence els_config warnings in PropEr suite
fabjan Feb 17, 2023
f1e265c
Increase halt_called wait timeout in PropEr suite
fabjan Feb 17, 2023
187acdb
Cleanup WIP comment in nonexisting_type test
fabjan Feb 17, 2023
3db8e76
Comment the version check in nonexisting_type test
fabjan Feb 18, 2023
13db044
Clean up useless leftover diffs
fabjan Feb 18, 2023
2769499
Fix halt_called timeout in PropEr suite
fabjan Feb 18, 2023
ca1aa6c
Only unload els_docs mock after cases that used it
fabjan Feb 20, 2023
27601e3
fmt
fabjan Feb 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
strategy:
matrix:
platform: [ubuntu-latest]
otp-version: [22, 23, 24]
otp-version: [23, 24, 25]
runs-on: ${{ matrix.platform }}
container:
image: erlang:${{ matrix.otp-version }}
Expand Down Expand Up @@ -89,7 +89,7 @@ jobs:
- name: Checkout
uses: actions/checkout@v2
- name: Install Erlang
run: choco install -y erlang --version 22.3
run: choco install -y erlang --version 23.3
- name: Install rebar3
run: choco install -y rebar3 --version 3.13.1
- name: Compile
Expand Down
6 changes: 4 additions & 2 deletions apps/els_lsp/src/els_docs.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ docs(_M, _POI) ->
-spec function_docs(application_type(), atom(), atom(), non_neg_integer()) ->
[els_markup_content:doc_entry()].
function_docs(Type, M, F, A) ->
case eep48_docs(function, M, F, A) of
%% call via ?MODULE to enable mocking in tests
case ?MODULE:eep48_docs(function, M, F, A) of
{ok, Docs} ->
[{text, Docs}];
{error, not_available} ->
Expand All @@ -126,7 +127,8 @@ function_docs(Type, M, F, A) ->
-spec type_docs(application_type(), atom(), atom(), non_neg_integer()) ->
[els_markup_content:doc_entry()].
type_docs(_Type, M, F, A) ->
case eep48_docs(type, M, F, A) of
%% call via ?MODULE to enable mocking in tests
case ?MODULE:eep48_docs(type, M, F, A) of
{ok, Docs} ->
[{text, Docs}];
{error, not_available} ->
Expand Down
13 changes: 11 additions & 2 deletions apps/els_lsp/test/els_completion_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1962,10 +1962,19 @@ select_completionitems(CompletionItems, Kind, Label) ->

has_eep48_edoc() ->
list_to_integer(erlang:system_info(otp_release)) >= 24.

has_eep48(Module) ->
case catch code:get_doc(Module) of
{ok, _} -> true;
_ -> false
{ok, {docs_v1, _, erlang, _, _, _, Docs}} ->
lists:any(
fun
({_, _, _, Doc, _}) when is_map(Doc) -> true;
({_, _, _, _, _}) -> false
end,
Docs
);
_ ->
false
end.

keywords() ->
Expand Down
57 changes: 50 additions & 7 deletions apps/els_lsp/test/els_hover_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ end_per_testcase(TestCase, Config) ->
%% Testcases
%%==============================================================================
local_call_no_args(Config) ->
%% this test is for the fallback render when no doc chunks are available
CleanupMock = mock_doc_chunks_unavailable(),
Uri = ?config(hover_docs_caller_uri, Config),
#{result := Result} = els_client:hover(Uri, 10, 7),
?assert(maps:is_key(contents, Result)),
Expand All @@ -94,9 +96,12 @@ local_call_no_args(Config) ->
value => Value
},
?assertEqual(Expected, Contents),
CleanupMock(),
ok.

local_call_with_args(Config) ->
%% this test is for the fallback render when no doc chunks are available
CleanupMock = mock_doc_chunks_unavailable(),
Uri = ?config(hover_docs_caller_uri, Config),
#{result := Result} = els_client:hover(Uri, 13, 7),
?assert(maps:is_key(contents, Result)),
Expand All @@ -117,9 +122,12 @@ local_call_with_args(Config) ->
value => Value
},
?assertEqual(Expected, Contents),
CleanupMock(),
ok.

remote_call_multiple_clauses(Config) ->
%% this test is for the fallback render when no doc chunks are available
CleanupMock = mock_doc_chunks_unavailable(),
Uri = ?config(hover_docs_caller_uri, Config),
#{result := Result} = els_client:hover(Uri, 16, 15),
?assert(maps:is_key(contents, Result)),
Expand All @@ -137,6 +145,7 @@ remote_call_multiple_clauses(Config) ->
value => Value
},
?assertEqual(Expected, Contents),
CleanupMock(),
ok.

local_call_edoc(Config) ->
Expand Down Expand Up @@ -226,6 +235,8 @@ remote_call_otp(Config) ->
ok.

local_fun_expression(Config) ->
%% this test is for the fallback render when no doc chunks are available
CleanupMock = mock_doc_chunks_unavailable(),
Uri = ?config(hover_docs_caller_uri, Config),
#{result := Result} = els_client:hover(Uri, 19, 5),
?assert(maps:is_key(contents, Result)),
Expand All @@ -246,9 +257,12 @@ local_fun_expression(Config) ->
value => Value
},
?assertEqual(Expected, Contents),
CleanupMock(),
ok.

remote_fun_expression(Config) ->
%% this test is for the fallback render when no doc chunks are available
CleanupMock = mock_doc_chunks_unavailable(),
Uri = ?config(hover_docs_caller_uri, Config),
#{result := Result} = els_client:hover(Uri, 20, 10),
?assert(maps:is_key(contents, Result)),
Expand All @@ -266,6 +280,7 @@ remote_fun_expression(Config) ->
value => Value
},
?assertEqual(Expected, Contents),
CleanupMock(),
ok.

edoc_definition(Config) ->
Expand Down Expand Up @@ -526,13 +541,15 @@ nonexisting_type(Config) ->
#{result := Result} = els_client:hover(Uri, 22, 15),
%% The spec for `j' is shown instead of the type docs.
Value =
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```"
Comment on lines -529 to -533
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

"```erlang\nj(_ :: doesnt:exist()) -> ok.\n```\n\n"
"---\n\n\n"
>>;
false ->
% els_eep48_docs:render returns {error, function_missing} for
% OTP versions under 25
_ ->
<<
"## j/1\n\n---\n\n```erlang\n\n j(_) \n\n```\n\n"
"```erlang\n-spec j(doesnt:exist()) -> ok.\n```"
Expand Down Expand Up @@ -560,10 +577,36 @@ nonexisting_module(Config) ->
?assertEqual(Expected, Result),
ok.

%%==============================================================================
%% Helpers
%%==============================================================================

%% @doc
%% Returns a function that can be used to unload the mock.
%% @end
mock_doc_chunks_unavailable() ->
meck:expect(
els_docs,
eep48_docs,
fun(_, _, _, _) -> {error, not_available} end
),
fun() ->
ok = meck:unload(els_docs)
end.

has_eep48_edoc() ->
list_to_integer(erlang:system_info(otp_release)) >= 24.

has_eep48(Module) ->
case catch code:get_doc(Module) of
{ok, _} -> true;
_ -> false
{ok, {docs_v1, _, erlang, _, _, _, Docs}} ->
lists:any(
fun
({_, _, _, Doc, _}) when is_map(Doc) -> true;
({_, _, _, _, _}) -> false
end,
Docs
);
_ ->
false
end.
5 changes: 4 additions & 1 deletion apps/els_lsp/test/prop_statem.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ exit_post(S, _Args, Res) ->
true -> 0;
false -> 1
end,
els_test_utils:wait_for(halt_called, 1000),
els_test_utils:wait_for(halt_called, 6400),
?assert(meck:called(els_utils, halt, [ExpectedExitCode])),
?assertMatch(ok, Res),
true.
Expand Down Expand Up @@ -376,6 +376,9 @@ setup() ->
application:ensure_all_started(els_lsp),
ConfigFile = filename:join([els_utils:system_tmp_dir(), "erlang_ls.config"]),
file:write_file(ConfigFile, <<"">>),
%% An empty config file makes the config loader log warnings,
%% we don't need to see them when running the tests.
logger:set_module_level(els_config, error),
ok.

%%==============================================================================
Expand Down