Skip to content

Conversation

@matthewdehaven
Copy link
Contributor

By moving warns-check.lisp into the shop-test-helper component, then adding a depends-on to shop-pddl-tests for that module. The other component which was depending on the warns-check, shop-internal-tests, already depends on the shop-test-helper component.

Addresses #138

By moving warns-check.lisp into the shop-test-helper component, then
adding a depends-on to shop-pddl-tests for that module. The other
component which was depending on the warns-check, shop-internal-tests,
already depends on the shop-test-helper component.
@rpgoldman
Copy link
Contributor

The version of fiveam that is included as SHOP submodule has fiveam:warns defined already. I have a suggestion for a minor refinement that I will post in the review.

Copy link
Contributor

@rpgoldman rpgoldman left a comment

Choose a reason for hiding this comment

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

If you could make that change and verify it, I will get this merged.

:pathname "tests/"
:components ((:file "common")))
:components ((:file "common")
(:file "warns-check")))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check to see if this macro is already defined by FiveAM (there's a pull request for it). So at the top of warns-check, add:

(eval-when (:compile-toplevel :load-toplevel :execute)
    (when (fboundp (uiop:intern* '#:warns :fiveam)) (pushnew :fiveam-warns features)))

#+(not fiveam-warns)
<warns definition>

If I knew that this was going to be added at a particular FiveAM version, I could do this in a cleaner way, but 🤷‍♂️

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 added the check before the warns definition here: ccb211d

Let me know if that's not what you are expecting and I can fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CI tests are failing after adding the check for existence of warns. At a glance, the test failure looks unrelated to the change, so I’m confused at the moment.

I’ll run the tests locally later today to see if I can replicate and debug the test failure.

There is an open pull request to add the `warns` check to fiveam. If
that pull request is merged, the `warns` check here should no longer
be used. The check added by this commit will ensure that the `warns`
defined in warns-check.lisp is only used if `warns` is not already
defined.

See lispci/fiveam#81
@rpgoldman
Copy link
Contributor

@matthewdehaven Checking master to see why the tests are failing here. Merge to come soon, promise!

@rpgoldman
Copy link
Contributor

Interesting: SBCL passes on master but fails on this branch. Investigating.

@rpgoldman
Copy link
Contributor

I have found what's wrong. Somehow the call to warns is not returning the value of the enclosed form, as it should. I will see if I can figure out why.

The tests were so big that it was hard to tell just what was failing.

Change this too-big test into a suite containing a number of sub-tests.
@rpgoldman rpgoldman merged commit 7730eac into master Aug 18, 2023
@rpgoldman rpgoldman deleted the issue-138-warns-check-dependency branch August 18, 2023 20:06
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.

4 participants