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

Getting it to work on ACAD 2017+ #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thelegendofbrian
Copy link
Contributor

@thelegendofbrian thelegendofbrian commented Aug 14, 2021

It didn't seem to work for ACAD 2018 or 2020 (tested on C3D vertical). The ACAD date variable was silently changed in ACAD 2017+, so it broke the timer functionality.

Known issues:

  • The test time outputs 0ms sometimes if the test takes under ~15ms, even if running the same test back to back. It seems to be related to ACAD not refreshing the counter often enough, though I'm not sure.

Example success:

ALUnit version 1.0
.......
Time: 62 ms

OK (7 tests run)

Example failure:

ALUnit version 1.0
......X
Time: 0 ms
1. round-num-test(NOT (nil)) returned T instead of nil.

FAILURES!!!
Tests run: 7, Failures: 1

Copy link
Owner

@jdsandifer jdsandifer left a comment

Choose a reason for hiding this comment

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

Regarding the first commit: It's been a long time since I worked on this and I don't have an AutoCAD environment now to test things, but are you sure you're correctly adding a list to each test when you create the tests?

The documentation for the defineTest function says that expressions should be a list. It seems like that should work correctly with the foreach. (And wouldn't work correctly with an eval.)

(Making defineTest automatically turn a single expression into a list or otherwise correctly handle a single expression might be a nice feature, but I think we need to be able to handle multiple expressions, too.)

Copy link
Owner

@jdsandifer jdsandifer left a comment

Choose a reason for hiding this comment

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

Nice work on the timing stuff! Please double-check how you're creating tests as I mentioned above and get back to me about whether that makes things work or not (so we can avoid unnecessary changes if possible).

@@ -304,7 +304,7 @@ SOFTWARE.
| *ALU:testSuites* - assoc list of test suites and a list of their tests
| *ALU:testsRun* - counter for total tests run in the current batch
| *ALU:failMessages* - list of messages to print after testing
| *ALU:startTime* - date number for start of testing
| *ALU:startTime* - milliseconds since boot time for start of testing
Copy link
Owner

Choose a reason for hiding this comment

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

This looks like good stuff, but I don't think it should go in this release file. Could you create a new release file for these changes? v1.1.0 sounds right to me as this is an important change, but shouldn't break anything, right?

@@ -22,7 +22,7 @@
| *ALU:testSuites* - assoc list of test suites and a list of their tests
| *ALU:testsRun* - counter for total tests run in the current batch
| *ALU:failMessages* - list of messages to print after testing
| *ALU:startTime* - date number for start of testing
| *ALU:startTime* - milliseconds since boot time for start of testing
Copy link
Owner

Choose a reason for hiding this comment

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

I can't test these timing change now, but this seems like a good approach. Nice work!

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