-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
WIP: Feature/calendar unit test #487
base: master
Are you sure you want to change the base?
Conversation
Make sure the tests still work next year :) |
It will fail soon I know. This is limited by current lib. We may need a way to tell the calendar info, when given a date |
Please consider adding test cases for the @statementreply , Since you're probably most familiar with the implementation details, could you please kindly give some insights if possible? |
…oes not work in LiFa, see one issue I just filed
Currently, 彼 functions takes a Unix timestamp (UTC), and converts to Chinese calendar date/time of local time, so they are affected by the runtime time zone setting. |
|
@statementreply Can you also help with the question below (a few functions like this did not work somehow)? https://github.com/LingDong-/wenyan-lang/issues/488 Thanks. |
Another failed test case for function "言百內數", can you help @statementreply . See the build logs below for error details |
The latest: 1 new function passed, 3 failed (may have to be fixed in lib), see build log for details. |
Done with calendar unit test suite. Please review/merge |
Thanks so much for the new tests! Just wondering about a couple things: Is there a reason the examples are not collected in one place but instead spanned across a bunch of files with just 1-2 lines, breaking the original structure of I propose that since the calendar involves many calculations similar to num2hanzi, we can create a file of similar structure to "wenyan" is styled without the hyphen in the middle, btw. Thanks! |
The reason why I wanted to separate some calendar test cases from examples is that when one test case fails inside the .wy file in examples it's very difficult to know the root cause. The new unit tests also print more info to the screen so you can tell what got tested and the status. I am also open to your suggestions so feel free to make the changes as you see fit. |
Understood. I'll look into it and make some fixes, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made a quick review. Please check it out. Thanks!
console.log("Entering function readOtherExample: x = " + x); | ||
return fs | ||
.readFileSync( | ||
path.resolve(__dirname, "../examples/.calendar" + x + ".wy"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem seems to from this.
- path.resolve(__dirname, "../examples/.calendar" + x + ".wy"),
+ path.resolve(__dirname, "../examples/calendar", x + ".wy"),
@@ -0,0 +1,26 @@ | |||
name: Node CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The npm t
should run all the tests. This extra action config seems to be not necessary.
}) | ||
} | ||
*/ | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also run npm run test:update
and include the snapshots to the commits.
To my knowledge, tests should not output anything when it passes. It only outputs when it failed. Too many outputs can overhaul the test result. |
@antfu. Thanks for the review! I think it might be better to test the calendar similar to how we test hanzi2num, perhaps like so (haven't tried it, just an idea): var assert = require("assert");
var { compile } = require("../src/parser");
var utils = require("../tools/utils")
var compile_options = {
romanizeIdentifiers: "none",
lib: utils.loadlib(),
logCallback: ()=>0,
}
describe("calendar", () => {
describe("言彼之時刻", () => {
it("should test calendar stuff", () => {
assert.strictEqual(
eval(compile("吾嘗觀「「曆法」」之書。方悟「言彼之時刻」之義。施「言彼之時刻」於四千七百一十四。書之", compile_options)),
"戌正一刻三分三十四秒"
);
});
});
describe("言彼之日時", () => {
it("should test more calendar stuff", () => {
assert.strictEqual(
eval(compile("吾嘗觀「「曆法」」之書。方悟「言彼之日時」之義。施「言彼之日時」於四千七百一十四。書之。", compile_options)),
"西元一九六九年己酉年十一月二十三日庚辰日戌正一刻三分三十四秒"
);
});
});
}); You're more familiar with the testing part, so please let me know if this approach is good :) |
@LingDong- Just think about the same thing! Stdlib definitely needs some better tests. Though I am a little bit too busy recently to work on this. For this approach we can write a simple utils to wrap the compile options and other stuff. Then we can focus on input/output when writing tests. I will make a PR for this later. |
@antfu Sounds great, thanks! No hurry ;) I can look into this too. |
Added unit test for calendar. It will also print meaningful info to the console such as: