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

coq-par-compile: use hash for ancestors #503

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

hendriktews
Copy link
Collaborator

The hash avoids an exponentially growing number of duplicates in
the ancestor collection. Fixes #499

The hash avoids an exponentially growing number of duplicates in
the ancestor collection. Fixes ProofGeneral#499
@hendriktews
Copy link
Collaborator Author

Where can I find something about the test that is failing here?

@erikmd
Copy link
Member

erikmd commented Jun 20, 2020

Hi @hendriktews, you can just click on the Checks tab of the PR.
But actually it seems the error is a spurious failure (the test 020_coq-test-definition fails with Coq 8.10 because at the time the assertion is checked, the expected text in *response* buffer is not there), so I guess we can just ignore that error!
(and I'll try to have a look on this later on to make the test more robust)

Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

@hendriktews albeit I'm not very elisp-hash-savvy, the PR LGTM and compiles fines… do you need that @RalfJung performs another test before the merge? or just when your patch will be available on MELPA.

@hendriktews
Copy link
Collaborator Author

Hi @hendriktews, you can just click on the Checks tab of the PR.

Yes, but there I only see that test (8.10, minimal) fails. Where can I find a description of the test that fails or its source code? Finding out that it is 020_coq-test-definition is already difficult, because this is somewhere hidden in the middle of the log.

I really appreciate that we have continuous integration now and even with automated tests! I am willing to investigate, when something fails on one of my RP's, but I don't really know how.

@hendriktews
Copy link
Collaborator Author

do you need that @RalfJung performs another test before the merge?

It would of course be nice, but I don't think it is required. I tested it on @RalfJung's repo and also tested it with an compilation error and coq-compile-keep-going...

@erikmd
Copy link
Member

erikmd commented Jun 22, 2020

@hendriktews

Yes, but there I only see that test (8.10, minimal) fails. Where can I find a description of the test that fails or its source code? Finding out that it is 020_coq-test-definition is already difficult, because this is somewhere hidden in the middle of the log.
I really appreciate that we have continuous integration now and even with automated tests! I am willing to investigate, when something fails on one of my RP's, but I don't really know how.

Yes, the interface of GitHub Actions logs is not extremely intuitive, but to summarize:

  • First click on the Checks tab − https://github.com/ProofGeneral/PG/pull/503/checks
  • Click on the workflow name, CI (on the top-left of the Checks page, cf. screenshot below)
    2020-06-22_22-48-29_Screenshot_PG_CI
  • You are now at page https://github.com/ProofGeneral/PG/actions/runs/141279705
  • You can see the name of the tests that failed (cf. screenshot below)
    2020-06-22_22-50-14_Screenshot_Problem_matcher
  • (If only one version of Coq is concerned, this might be a spurious failure)
  • If need be, you can grep the test name in that file to get the code: https://github.com/ProofGeneral/PG/blob/master/ci/coq-tests.el

    PG/ci/coq-tests.el

    Lines 192 to 200 in 5cc2316

    (ert-deftest 020_coq-test-definition ()
    "Test *response* output after asserting a Definition."
    (coq-fixture-on-file
    (coq-test-full-path "test_stepwise.v")
    (lambda ()
    (coq-test-goto-before "(*test-definition*)")
    (proof-goto-point)
    (proof-shell-wait)
    (coq-should-buffer-string "trois is defined"))))
  • For further details, you can either:
    • Re-run the test locally in batch mode by doing cd .../PG/ci && ./test.sh (no docker binary required)
    • Re-run the test locally in interactive mode with − cf. New hook for early prompt/output analyzis. #495 (comment) :
      C-x C-f ci/coq-tests.el RETM-: (progn (load-file "coq-tests.el") (call-interactively #'ert)) RET
    • Or directly browse the GitHub Actions log, by clicking successively on:
      2020-06-22_22-56-40_Screenshot_Fail_1

      2020-06-22_22-56-56_Screenshot_Fail_2

      2020-06-22_22-57-31_Screenshot_Fail_3

      2020-06-22_22-57-56_Screenshot_Fail_4
      (the main error line is automatically recognized (by a custom regexp) and underlined in red − this is the so-called Problem Matcher feature of GitHub Action)

Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

I tested it on @RalfJung's repo and also tested it with an compilation error and coq-compile-keep-going...

OK, thanks @hendriktews! 👍

So if no one objects, let's merge your PR on tomorrow Tuesday

@erikmd erikmd merged commit 03e427a into ProofGeneral:master Jun 23, 2020
@hendriktews hendriktews deleted the issue-499-ancestor-append branch January 21, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

emacs eats up all my RAM while processing imports
2 participants