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

refactor(tests): simplify test code #363

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

lxl66566
Copy link
Contributor

@lxl66566 lxl66566 commented Jul 27, 2024

changes:

  • merge validity_test and stdlib_test logic into an inner function script_test: the test logic are same, it's better not to use same code
  • rewrite extract_output to more convenient form (with test)

@Mte90 Mte90 requested review from Ph0enixKM and b1ek July 27, 2024 09:52
@Mte90
Copy link
Member

Mte90 commented Jul 27, 2024

I see that #362 include some stuff that are in this PR.
So what is the right one to review? maybe the other one can be closed?

@b1ek
Copy link
Member

b1ek commented Jul 27, 2024

I see that #362 include some stuff that are in this PR. So what is the right one to review? maybe the other one can be closed?

this PR depends on the other one, means the author has included that PR's commits in this one. just ignore it for now and wait for 362 to be merged

@Mte90
Copy link
Member

Mte90 commented Jul 27, 2024

Or we can merge this one as include more stuff.

@lxl66566
Copy link
Contributor Author

i rebased this pr, it's not related to another pr now, that may be more convenient

@Mte90 Mte90 self-requested a review July 29, 2024 09:17
@Mte90
Copy link
Member

Mte90 commented Jul 29, 2024

@lxl66566 can you fix the conflicts?

@lxl66566
Copy link
Contributor Author

@lxl66566 can you fix the conflicts?

rebased

@Ph0enixKM
Copy link
Member

I'd rather not. When changing some internal functionality - I want to see all the validity checks green before I check the stdlib.

  • The standard library tests are testing the functioanlity of the standard library functions.
  • The validity tests check whether the compiler works properly.

Why keep the separate?

  1. To separate the library noise from the compiler itself - I want to check if what I changed messed up with the core language itself and I don't want to worry about some library functions fail because I'm currently on macOS and some command relies on Linux version of the command API.
  2. To have a way to run just the compiler checks - sometimes library tests can fail and that's okay (some commands might be not installed on the system). Compiler tests cannot fail at all.

Do you agree with me?

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

I think that we shouldn't move validity (the compiler validity) tests to the stdlib (standard library) tests.

@lxl66566
Copy link
Contributor Author

lxl66566 commented Jul 29, 2024

I think that we shouldn't move validity (the compiler validity) tests to the stdlib (standard library) tests.

I got you. I think we can put the test logic in script_test(input) in mod.rs, and just use stdlib_test(input), validity_test(input) as two wrappers to call inner script_test(input). That can avoid simply repeating the same code.

And then, put stdlib_test(input) and validity_test(input) in two files, so you can pick any one you like to test.

How do you think of that?

@KrosFire
Copy link
Member

Taking this opportunity. Why don't we start using insta? We wouldn't have create output txt files. We would just approve generated ones

@Mte90
Copy link
Member

Mte90 commented Jul 30, 2024

Taking this opportunity. Why don't we start using insta? We wouldn't have create output txt files. We would just approve generated ones

Right now with the latest changes there are just a couples of .txt files as we removed the majority.

@Mte90
Copy link
Member

Mte90 commented Jul 30, 2024

Can you align with the latest changes?
So @Ph0enixKM can review it :-)

@lxl66566
Copy link
Contributor Author

master now in #300 didn't pass the test.

@Mte90
Copy link
Member

Mte90 commented Jul 31, 2024

A pr fixed that issue.

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Bug detected

src/tests/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Phoenix Himself <[email protected]>
@Mte90 Mte90 requested a review from Ph0enixKM August 1, 2024 10:36
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

All good now

@Ph0enixKM Ph0enixKM merged commit 969e614 into amber-lang:master Aug 1, 2024
1 check passed
@lxl66566 lxl66566 deleted the refactor-test branch August 1, 2024 11:05
Mte90 added a commit to Mte90/Amber that referenced this pull request Sep 19, 2024
* refactor(tests): simplify test code

Signed-off-by: lxl66566 <[email protected]>

* Update src/tests/mod.rs

Co-authored-by: Phoenix Himself <[email protected]>

---------

Signed-off-by: lxl66566 <[email protected]>
Co-authored-by: Daniele Scasciafratte <[email protected]>
Co-authored-by: Phoenix Himself <[email protected]>
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.

5 participants