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

WIP: Feature/calendar unit test #487

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

Conversation

kaiyuan01
Copy link
Contributor

Added unit test for calendar. It will also print meaningful info to the console such as:

image

@LingDong-
Copy link
Member

Make sure the tests still work next year :)

@kaiyuan01
Copy link
Contributor Author

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

@LingDong-
Copy link
Member

Please consider adding test cases for the versions (e.g. 彼年何年) instead. These should be current-date-agnostic. The versions directly call versions, so their correctness is trivial to prove when pass the tests.

@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
@kaiyuan01
Copy link
Contributor Author

kaiyuan01 commented Jan 4, 2020

Current test result:

image

@statementreply
Copy link
Contributor

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.

@kaiyuan01
Copy link
Contributor Author

施「彼年何年」於四千七百一十六
Should we name it "彼時何年" instead of "彼年何年" to avoid confusion since the input is in secs instead of years? https://github.com/LingDong-/wenyan-lang/issues/489

@kaiyuan01
Copy link
Contributor Author

@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.

@kaiyuan01
Copy link
Contributor Author

kaiyuan01 commented Jan 5, 2020

Another failed test case for function "言百內數", can you help @statementreply . See the build logs below for error details

@kaiyuan01
Copy link
Contributor Author

The latest: 1 new function passed, 3 failed (may have to be fixed in lib), see build log for details.

@kaiyuan01
Copy link
Contributor Author

Done with calendar unit test suite. Please review/merge

@LingDong-
Copy link
Member

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 ./examples folder? Is there a reason examples.test.js is not used, but instead an almost identical file is duplicated with the same contents just to test the calendar? Has the problem of timezones in Unix timestamp been somehow addressed in the tests?

I propose that since the calendar involves many calculations similar to num2hanzi, we can create a file of similar structure to hanzi2num.test.js. This way we can also do some tricks in the JS to perhaps work around the fact that the functions depend on the current time & timezone. A compile step has to be involved to do so however, unlike hanzi2num which is in JS. If you're uncomfortable writing that I'm more than happy to help!

"wenyan" is styled without the hyphen in the middle, btw.

Thanks!

@kaiyuan01
Copy link
Contributor Author

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.

@LingDong-
Copy link
Member

Understood. I'll look into it and make some fixes, thanks!

Copy link
Member

@antfu antfu left a 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"),
Copy link
Member

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
Copy link
Member

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.

})
}
*/
});
Copy link
Member

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.

@antfu
Copy link
Member

antfu commented Jan 7, 2020

@kaiyuan01

The new unit tests also print more info to the screen so you can tell what got tested and the status.

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.

@LingDong-
Copy link
Member

LingDong- commented Jan 7, 2020

@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 :)

@antfu
Copy link
Member

antfu commented Jan 7, 2020

@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.

@LingDong-
Copy link
Member

LingDong- commented Jan 7, 2020

@antfu Sounds great, thanks! No hurry ;) I can look into this too.

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.

None yet

4 participants