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

Basic Automated Test Harness #26

Closed
wants to merge 18 commits into from

Conversation

PrimaryCanary
Copy link
Contributor

@PrimaryCanary PrimaryCanary commented Jan 19, 2021

Here is a very simple framework for ensuring all the formatters properly configured. Currently, it has a small issue in that it doesn't actually work.

Each test consists of two files of the format formatters/mode-it-should-be-active-in/formatter.ext and formatters/mode-it-should-be-active-in/formatter.ext.formatted. The former is unformatted code while the latter is the expected output after running the formatter. test-apheleia.el should (but doesn't) return the result of calling the given formatter on the unformatted code to stdout.

All output from test-apheleia.el should be coming from the apheleia--run-formatters function. I think any stdout is being smothered because it's called from the async inferior process. How would I make test-apheleia.el return the formatted buffer to stdout?

TODO:

  • Make test-apheleia.el work.
  • Ensure all tests contain the proper text.
  • Add supported modes and formatters table to README.
  • Add test to ensure README table matches apheleia-modes-alist and apheleia-formatters.
  • Run tests from Docker container.
  • Add a test target in the Makefile.
  • Add package-lint target in the Makefile.
  • Run the tests in CI.

@PrimaryCanary PrimaryCanary changed the title Basic Automated Test Harness [DRAFT] Basic Automated Test Harness Jan 19, 2021
Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

I love how simple it is to add a test, just adding the two files and no extra busywork required!

scripts/docker-install.bash Outdated Show resolved Hide resolved
scripts/docker-install.bash Outdated Show resolved Hide resolved
test/formatters/web-mode/prettier-babel.html.formatted Outdated Show resolved Hide resolved
test/test-apheleia.el Outdated Show resolved Hide resolved
#!/usr/bin/env bash
set -euo pipefail

find formatters/ -type f,l ! -name "*.formatted" | while read f;
Copy link
Member

Choose a reason for hiding this comment

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

-type f,l

Do we want to find symlinks, or just files?

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've used symlinks for modes that are likely to be working on the same code with the same formatter e.g. rust-mode and rustic-mode both work on Rust and likely use rustfmt. I can strengthen the requirement if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, that makes sense.

test/test-apheleia.el Outdated Show resolved Hide resolved
Sifus added 10 commits January 20, 2021 13:23
Since the callback of apheleia--run-formatter is run in an asynchronous
process, all output to stdout was being handled by the inferior process.
Output must be retrieved in the main process with accept-process-output.

Additionally, any formatter that uses the file directive will not run if
buffer doesn't match the file on disk. with-temp-buffer doesn't have an
associated file so most formatters wouldn't run.
Ensures apheleia-formatters and apheleia-mode-alist have no missing
or incorrect entries in the README.
@PrimaryCanary PrimaryCanary marked this pull request as draft January 21, 2021 07:51
@PrimaryCanary
Copy link
Contributor Author

PrimaryCanary commented Feb 2, 2021

The good:

  • The Docker container successfully builds and runs all formatters.
  • The tests will run in CI.

The bad:

  • There are currently two formatters building from source. I couldn't find a binary for ocamlformat and the Brittany binary Didn't Work On My MachineTM.
  • The container builds very slowly as a result. We might have to use your expertise to speed up CI and builds.
  • No point tests which would make testing and fixing Point is sometimes moved small distances #2 easier.

The ugly:

  • I haven't tested the CI settings because I don't use CircleCI. It looks like it will work.

If none of those issues are too big, I'd say this is ready to merge.

@PrimaryCanary PrimaryCanary changed the title [DRAFT] Basic Automated Test Harness Basic Automated Test Harness Feb 2, 2021
@PrimaryCanary PrimaryCanary marked this pull request as ready for review February 2, 2021 05:51
@raxod502
Copy link
Member

I fixed things up so CI will run in only 2-3 minutes, and made various other improvements. The main thing remaining is that there seems to be something wrong with json-mode in Emacs 25, do you think you could take a look at that?

@raxod502
Copy link
Member

This thread is being closed automatically by Tidier because it is labeled with "waiting on response" and has not seen any activity for 90 days. But don't worry—if you have any information that might advance the discussion, leave a comment and I will be happy to reopen the thread :)

@afrepues afrepues mentioned this pull request Aug 26, 2021
@raxod502
Copy link
Member

raxod502 commented Jan 5, 2022

I wrote my own version of this in #72, partly drawing from the code here. The only things missing are package-lint integration, and the auto-generated table of formatters in the README.

@raxod502 raxod502 mentioned this pull request Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants