Fix tests and implement testing in CI #30
Conversation
Fixes:
tests/purescript-sort-imports-tests.el:31:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:41:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:51:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:61:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:73:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:86:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:99:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
tests/purescript-sort-imports-tests.el:118:14: Warning: ‘goto-line’ is for interactive use only; use ‘forward-line’ instead.
Fixes:
tests/purescript-str-tests.el:1:1: Error: file has no ‘lexical-binding’ directive on its first line
The code that's being removed here made no sense. It is a `check`
rule, and it did the following things:
1. For every `.el` file it was searching its `tests.el` counterpart.
Which doesn't exist.
2. It was checking the correctness of `declare-function`s. Which would
be fine, wasn't it for the fact the project has zero
`declare-function`s.
3. It was checking that `ert` exists, which it does on all supported
Emacs versions.
4. It was removing .elc files before running the tests. Why? 🤷♂️
Replace everything with a single `test` rule which simply loads the
test files and runs the tests.
Besides being actually useful, this also improves running time as:
Initial state | Before | After |
Non-compiled | 2.177 | 1.614 |
Compiled | 2.182 | 0.340 |
|
New commit: Compile .el files at O(1) instead of O(n)
The older code was running Emacs separately for each .el file. Change
the code so that all .el files are passed at once.
This improves build time by x5:
* before: 1.272 sec
* after: 0.257 sec |
|
@purcell hello, it is possible to get some review here? I'm currently blocked on this for indentation testing that I'm trying to add, because testing would have to be fixed in the first place. |
Makefile
Outdated
|
|
||
| %.elc: %.el | ||
| .el.elc: |
There was a problem hiding this comment.
Should this target be called something like bytecode and included in .PHONY?
There was a problem hiding this comment.
This line can't be arbitrarily named, this is a special syntax that says "given *.el files, use code below to create *.elc from them". If we go up the stack, this rule is relied upon by compile, which does reside in .PHONY.
But actually, as far as I understand compile should be removed from .PHONY. "phony rules" are used to declare rules without dependencies, like "tests" which you want to always run anew. compile on the other hand creates bytecode, which there's no point to recreate unless it's outdated, so compile shouldn't be a phony rule.
There was a problem hiding this comment.
this is a special syntax that says
Yeah, that's what the % syntax means, but not what the replacement in this PR means, right?
"phony rules" are used to declare rules without dependencies, like "tests" which you want to always run anew
No, not at all: phony rules are rules that don't create an output file with the same name as the rule.
There was a problem hiding this comment.
Yeah, that's what the
%syntax means, but not what the replacement in this PR means, right?
Ah, no, they both mean the same, just .el.elc is shorter. I found it in Emacs sources. I can remove it if you want. I just thought, given they both mean same thing, the .el.elc variation may be easier to read.
No, not at all: phony rules are rules that don't create an output file with the same name as the rule.
Right, though it may be better to say that they don't depend on a file with same name, even if such is created. You are right, now that I think compile should be a PHONY rule. Either way, it is one, so there's nothing to do for me 😊
There was a problem hiding this comment.
So, I dug into GNU Make documentation and found something weird. This syntax is documented here and apparently they deprecated it despite being easier to read. *sigh* People sometimes make very strange decisions…
Anyway, I guess I'll remove this change.
The older code was running Emacs separately for each .el file. Change the code so that all .el files are passed at once. This improves build time by x5: * before: 1.272 sec * after: 0.257 sec Also simplify the rule declaration.
|
Oh, yeah, build on Emacs master does not succeed now, it is being fixed in a separate PR I sent 5 days ago. |
|
Will close and re-open to trigger CI, now that #32 is merged. |
|
Merged now, thanks. |
The project have had some tests, which even though were apparently from Haskell time, they still seem to be testing some code. The imports sorting probably requires some syntax changes (e.g. to stop accounting for
qualifiedkeyword), but the algo is simple and somebody may still be using it.Now, the interesting part is that the project has
checkMakefile rule (one that I'm removing here). I have no slightest idea what it was written for. As described in the commit, it did the following 4 things:.elfile it was searching itstests.elcounterpart. Which doesn't exist.declare-functions. Which would be fine, wasn't it for the fact the project has zerodeclare-functions.ertexists, which it does on all supported Emacs versions.Replacing
checkwith much simplertestrule which just runs tests, besides being actually useful, also improves running time as follows: