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.

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

Improved resilience of formatter_for #738

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

scohen
Copy link
Contributor

@scohen scohen commented Oct 4, 2022

It's possible that the call to Mix.Tasks.Format.formatter_for_file can cause a compile or syntax errors. Because these types of errors don't have a message key, the attempt to log them will fail, and the server will crash.

This PR uses Exception.message to build the message text, which should work on any exceptions.

It's possible that the call to `Mix.Tasks.Format.formatter_for_file`
can cause a compile or syntax errors. Because these types of errors
don't have a `message` key, the attempt to log them will fail, and the
server will crash.

This PR uses `Exception.message` to build the message text, which
should work on any exceptions.
@scohen
Copy link
Contributor Author

scohen commented Oct 4, 2022

@lukaszsamson With respect to patch:
You pointed out it's another library, and that it prevents async: true.
I tried to exercise the syntax / compile errors "naturally" by building a project structure, and then placing an invalid .formatter.exs in there. This worked, but was easily 30 lines of code, and because I had to cd into the directory to get the call to Mix.Tasks.Format.formatter_for_file to recognize the .formatter.exs, the test was fragile, and was also not capable of async anyways. Even isolating the test in another file (as I have done above) didn't fix the problems that cd caused.

I would have been able to pass an option to formatter_for_file, to explicitly state which .formatter.exs to use, but formatter_for doesn't allow an option to be passed (and I'm pretty sure it's not reasonable to allow that anyways).

The options before me are:

  1. Leave this untested.
  2. Add an opts keyword list to formatter_for just for testing.
  3. Add patch

1 is not an option, because that's how we got into this situation
2 means that the testing code takes a different path than the actual code, which means the actual code isn't really getting tested properly.
3 means we add a library

I think 3 is the best option because it's a small, focused, high-quality library that makes these types of tests much easier to write, which is a good thing. Careful use of patching allows hard-to-exercise code paths testable with only a few lines. It's also a test-only dependency and has not runtime impact.

If you have a better suggestion than patch, I'm all ears.

@scohen scohen changed the title Improved resiliency of formatter_for Improved resilience of formatter_for Oct 4, 2022
@lukaszsamson
Copy link
Collaborator

OK, I'm convinced that option 3 is the one to go here. Especially if async: false tests are separated.

@lukaszsamson lukaszsamson merged commit 17ee361 into elixir-lsp:master Oct 6, 2022
@scohen
Copy link
Contributor Author

scohen commented Oct 6, 2022

yay! Thank you, @lukaszsamson

@lukaszsamson
Copy link
Collaborator

lukaszsamson commented Oct 12, 2022

@scohen are you aware if patch is working correctly on older erlang/elixir combos? Some tests are failing on 1.11 and 1.12, see https://github.com/elixir-lsp/elixir-ls/actions/runs/3229860195/jobs/5287630105. It states that elixir >= 1.7 is supported

@scohen
Copy link
Contributor Author

scohen commented Oct 14, 2022

@lukaszsamson i talked to the author, and it was developed under 1.11 and 1.12, so it should work. If the problem persists, he asked that you open an issue and he’s be happy to help fix it.

@scohen
Copy link
Contributor Author

scohen commented Oct 14, 2022

It's also interesting that the error seems to indicate that the patch isn't being applied.

@scohen
Copy link
Contributor Author

scohen commented Oct 14, 2022

@lukaszsamson I figured it out, see #749

@scohen scohen deleted the scohen/sourcefile-formatter branch November 7, 2022 19:27
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.

2 participants