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

Use just [rstest] to generate test. #42

Closed
svenstaro opened this issue Jun 28, 2019 · 10 comments
Closed

Use just [rstest] to generate test. #42

svenstaro opened this issue Jun 28, 2019 · 10 comments
Milestone

Comments

@svenstaro
Copy link
Contributor

My macro knowledge is pretty poor so please excuse my ignorance, but I think it'd be conceptually and syntactically to use have [rstest] which may also be optionally parametrized. What do you think?

@la10736
Copy link
Owner

la10736 commented Jun 28, 2019

Should work... maybe I can just remove rstest_parametrize() case and fall back in standard case if no cases provided.

Ok, I'll do some tests. If I don't find any problem I'll deprecate rstest_parametrize() in the next release (0.4.0) and then remove it in 0.5.0.

@la10736 la10736 changed the title Why not just have [rstest] instead of [rstest] and [rstest_parametrize]? Use just [rstest] to generate test. Jun 28, 2019
@la10736
Copy link
Owner

la10736 commented Jun 28, 2019

By [rstest] we can cover all cases:

  • simple identity like name means I should give it in case()
  • identity => [v1,.., vn] like name => [1,2,3] means matrix arguments
  • no referenced identity resolved by fixture search

For instance in the same test you can have a parameter from list and other 2 from cases: in this case will run all cases for each value in list.

@KSXGitHub
Copy link

KSXGitHub commented Jun 28, 2019

I don't think => for matrix would not be clear enough. My suggestion is to use matrix for matrix in combination of case for individual cases.

This following snippet

#[rstest(
  params(foo, bar),
  matrix(
    foo => [0, 1, 2],
    bar => ['a', 'b'],
  ),
  case(8, 'f'),
)]
fn mytest(foo: i32, bar: char) {}

should be equivalent to

#[rstest_parametrize(
  foo, bar,
  case(0, 'a'),
  case(1, 'a'),
  case(2, 'a'),
  case(0, 'b'),
  case(1, 'b'),
  case(2, 'b'),
  case(8, 'f'),
)]
fn mytest(foo: i32, bar: char) {}

I not sure if this syntax is good enough.

@la10736
Copy link
Owner

la10736 commented Jun 28, 2019

Ok, I'll think about this even if my idea is that in test writing you should remove all boilerplate that you can even if the syntax become less explicit.

@KSXGitHub
Copy link

you should remove all boilerplate that you can even if the syntax become less explicit.

Rustfmt has a problem with current rstest_parametrize: cargo fmt will turn the following code

#[rstest_parametrize(
  foo, bar, baz,
  case(0, 'a', 'A'),
  case(1, 'b', 'B'),
)
fn mytest (foo: u32, baz: char, baz: char) {}

into this

#[rstest_parametrize(
  foo,
  bar,
  baz,
  case(0, 'a', 'A'),
  case(1, 'b', 'B'),
)
fn mytest (foo: u32, baz: char, baz: char) {}

This is why I think params() (or a pair of parenthesis at the very least) is necessary.

@la10736
Copy link
Owner

la10736 commented Jul 1, 2019

Hi @KSXGitHub, I don't think that how rustfmt works on rstest_parametrize is a good argumentto choose the syntax. Anyway the behavior that you describe depends on rustfmt configurations:

on my machine use rustfmt without any configuration change it in

#[rstest_parametrize(foo, bar, baz, case(0, 'a', 'A'), case(1, 'b', 'B'))]
fn mytest(foo: u32, bar: char, baz: char) {}

Anyway what we are doing here is to build a new DSL and we cannot write DSL according to Attribute, Meta and NestedMeta rustfmt formatting choices. What we can do is to open a ticket (and maybe a pull request) to extend [rustfmt::skip::macros()] attribute to skip procedural macros too.

I took a look to rustfmt code and should be simple to implement it. In the meantime we can use #[rustfmt::skip] as workaround:

#[rustfmt::skip]
#[rstest_parametrize(
  foo, bar, baz,
  case(0, 'a', 'A'),
  case(1, 'b', 'B'),
)]
fn mytest (foo: u32, bar: char, baz: char) {}

Or use a small comment to do the trick

#[rstest_parametrize( // This prevent formatting
  foo, bar, baz,
  case(0, 'a', 'A'),
  case(1, 'b', 'B'),
)]
fn mytest (foo: u32, bar: char, baz: char) {}

@la10736
Copy link
Owner

la10736 commented Jul 2, 2019

Maybe by use rust-lang/rust#60406 we can improve the syntax a lot by move argument related data near the argument instead try to express it external attribute.

@la10736
Copy link
Owner

la10736 commented Jul 4, 2019

Just for the record: rust-lang/rustfmt#3665 will introduce rustfmt::skip::attributes() attribute both at item and module level. So we can use it to skip formatting in rstest's attribute. Unfortunately we can do it at rstest crate but should be the user that add this rustfmt directive to his code.

@la10736 la10736 added this to the 0.5 milestone Sep 29, 2019
@la10736
Copy link
Owner

la10736 commented Oct 5, 2019

I'm staring to work on it (moved on 0.5 release)

TODO:

  • integrate parametrize
    • first happy pathintegration test
    • parsing
    • rendedering (if some cases)
    • error if indicate some args but no cases
    • move all integration tests in rstest module
    • route parametrize to rstest
    • clean up tests
  • integrate matrix
    • first happy path integration test
    • parsing
    • Add tests for parsing both matrix, cases and fixture
    • Remove matrix parse module and move ValueList in its own module
    • rendedering (if some cases)
    • move all integration tests in rstest module
    • route matrix to rstest
    • clean up tests
  • cross cases
    • test happy path all together
    • implement logic for handling all cross cases
    • write unit tests for cross cases
    • refactor (wrap by module)
    • check tests about handling error (multiple cross args)
    • redirect rstest_parametrize and rstest_matrix docs to rstest
  • Unsorted
    • Add literal unit test for Fixture

la10736 added a commit that referenced this issue Oct 5, 2019
la10736 added a commit that referenced this issue Oct 5, 2019
la10736 added a commit that referenced this issue Oct 6, 2019
la10736 added a commit that referenced this issue Oct 6, 2019
la10736 added a commit that referenced this issue Oct 6, 2019
la10736 added a commit that referenced this issue Oct 6, 2019
la10736 added a commit that referenced this issue Oct 13, 2019
la10736 added a commit that referenced this issue Oct 13, 2019
la10736 added a commit that referenced this issue Oct 14, 2019
la10736 added a commit that referenced this issue Oct 20, 2019
la10736 added a commit that referenced this issue Oct 20, 2019
la10736 added a commit that referenced this issue Oct 21, 2019
la10736 added a commit that referenced this issue Oct 22, 2019
la10736 added a commit that referenced this issue Oct 23, 2019
la10736 added a commit that referenced this issue Oct 24, 2019
la10736 added a commit that referenced this issue Oct 24, 2019
la10736 added a commit that referenced this issue Oct 27, 2019
@la10736
Copy link
Owner

la10736 commented Oct 27, 2019

Implementation and tests are done. Docs moved in other ticket : see #67

@la10736 la10736 closed this as completed Oct 27, 2019
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

No branches or pull requests

3 participants