Skip to content

TEST_MATRIX feature#685

Merged
mvandervoord merged 4 commits intoThrowTheSwitch:masterfrom
jonhenneberg:test_matix_feature
Aug 13, 2023
Merged

TEST_MATRIX feature#685
mvandervoord merged 4 commits intoThrowTheSwitch:masterfrom
jonhenneberg:test_matix_feature

Conversation

@jonhenneberg
Copy link

As an analog for TEST_CASE(...) & TEST_RANGE(...), you can use TEST_MATRIX(...) for defining custom test cases.

TEST_MATRIX macro parsing can be enabled together with TEST_RANGE

Based on logic from Adding test matrix feature #599, but updated to match newer code, and added tests and documentation

Added matrix option for parameterization that generates cases based on
the product of the given arguments.
@jonhenneberg
Copy link
Author

@mvandervoord Do you have any idea on why my test is failing?
When running the tests locally (Windows 11, x64) everything passes, and I'm not well versed in rake to understand the error it's throwing.

@mvandervoord
Copy link
Member

mvandervoord commented Jul 14, 2023

Interesting. That's a new one for me. Searching a bit suggests that it's the compiler noticing we have a buffer overflow... I've not dug into it to see where? The rake error is dumb, I need to catch/cleanup that error message, but that's not on you. Right above the rake crash is the message from the tools being called, though: *** stack smashing detected ***: terminated. That's the one I describe above.

@jonhenneberg
Copy link
Author

Found the mistake, I had made some changes in one test where I forgot to define the size of an array,
I was able to reproduce the error by running in ubuntu, seems like it has some guard on the stack

@AJIOB
Copy link
Contributor

AJIOB commented Jul 15, 2023

What about adding TEST_MATRIX logic as a part of TEST_RANGE? With another open/close brackets (for example with {}).

It will be nice to combine range & matrix generators together (part of the arguments may use range generatore, another one may use matrix generators).

@AJIOB
Copy link
Contributor

AJIOB commented Jul 15, 2023

P.S. You are really awesome to actualize that feature. I had no time to do it myself.

enum_idx = (enum_idx + 1) % 3;
}
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

What about force adding empty line in the end for generating less changes in future while adding new tests?

Copy link
Author

Choose a reason for hiding this comment

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

I can do that, I have tools that ensure the formatting of my files 😀

1.2f,
2.3f,
3.4f,
}; No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

One more point

end

when "MATRIX"
single_arg_regex_string = /(?:(?:"(?:\\"|[^\\])*?")+|(?:'\\?.')+|(?:[^\s\]\["'\,]|\[[\d\S_-]+\])+)/.source
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, I split parts of single argument regex for readability & easily improving every regex part independently.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I noticed that in your PR, for some reason separating the elements makes it visualize how the regex will process the command.
I normally only do it if I need to reuse parts of the query, so I only have one place to maintain

@AJIOB
Copy link
Contributor

AJIOB commented Jul 15, 2023

@mvandervoord, what about adding that feature to 2.6 release (PR #656) too, if it is ready for that time?

@jonhenneberg
Copy link
Author

jonhenneberg commented Jul 15, 2023

What about adding TEST_MATRIX logic as a part of TEST_RANGE? With another open/close brackets (for example with {}).

It will be nice to combine range & matrix generators together (part of the arguments may use range generatore, another one may use matrix generators).

Should not be too difficult to do, unsure if we want the logic separated or not. Personally I've never had the need for testing ranges in the way they're used in TEST_RANGE`, as I normally test in the limits so if I know the code should do something at input 150, I might test 0, 149, 150, and 255.

I just used the format from your PR as a stepping off point, hence the separation. When I initially though about the how I would solve it, before coming across the old PR, I was thinking of doing some keywords to the normal TEST_CASE, something like TEST_CASE(12, MATRIX<1, 2, 7>, RANGE<1,3,1>)

@AJIOB
Copy link
Contributor

AJIOB commented Jul 16, 2023

What about adding TEST_MATRIX logic as a part of TEST_RANGE? With another open/close brackets (for example with {}).
It will be nice to combine range & matrix generators together (part of the arguments may use range generatore, another one may use matrix generators).

Should not be too difficult to do, unsure if we want the logic separated or not. Personally I've never had the need for testing ranges in the way they're used in TEST_RANGE`, as I normally test in the limits so if I know the code should do something at input 150, I might test 0, 149, 150, and 255.

I just used the format from your PR as a stepping off point, hence the separation. When I initially though about the how I would solve it, before coming across the old PR, I was thinking of doing some keywords to the normal TEST_CASE, something like TEST_CASE(12, MATRIX<1, 2, 7>, RANGE<1,3,1>)

Single-element ranges will be mapped to single TEST_CASE.

That is why it is not required to merge TEST_CASE &TEST_RANGE.

IMO, we should save TEST_MATRIX as a backward compatible API. We should reuse the code for parsing TEST_MARTIXes if we add that feature to the TEST_RANGEs.

@jonhenneberg
Copy link
Author

What about adding TEST_MATRIX logic as a part of TEST_RANGE? With another open/close brackets (for example with {}).
It will be nice to combine range & matrix generators together (part of the arguments may use range generatore, another one may use matrix generators).

Should not be too difficult to do, unsure if we want the logic separated or not. Personally I've never had the need for testing ranges in the way they're used in TEST_RANGE, as I normally test in the limits so if I know the code should do something at input 150, I might test 0, 149, 150, and 255. I just used the format from your PR as a stepping off point, hence the separation. When I initially though about the how I would solve it, before coming across the old PR, I was thinking of doing some keywords to the normal TEST_CASE, something like TEST_CASE(12, MATRIX<1, 2, 7>, RANGE<1,3,1>)`

Single-element ranges will be mapped to single TEST_CASE.

That is why it is not required to merge TEST_CASE &TEST_RANGE.

IMO, we should save TEST_MATRIX as a backward compatible API. We should reuse the code for parsing TEST_MARTIXes if we add that feature to the TEST_RANGEs.

I'll give it a try, and see if can can get the logic to look sensible, don't think it would be too hard

@jonhenneberg
Copy link
Author

@AJIOB I have an issue with using { and } is that they get scrubbed earlier in the logic, I assume to remove the content of functions, so would have to find some other way to represent a matrix in the range, and doubt ( and ) would be either, and with <, >, [ , and ] I'm running out of ideas for natural grouping indicators.

lines = source_scrubbed.split(/(^\s*\#.*$) | (;|\{|\}) /x)                     # Treat preprocessor directives as a logical line. Match ;, {, and } as end of lines
                           .map { |line| line.gsub(substring_unre, substring_unsubs) } # unhide the problematic characters previously removed

@AJIOB
Copy link
Contributor

AJIOB commented Jul 17, 2023

@jonhenneberg, I think we can add it in a future, when it is required.

Thank you for investigation!

@AJIOB
Copy link
Contributor

AJIOB commented Jul 29, 2023

@mvandervoord, what about merging this feature?

@mvandervoord mvandervoord merged commit 20bb435 into ThrowTheSwitch:master Aug 13, 2023
@AJIOB AJIOB mentioned this pull request Aug 14, 2023
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