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

Document where test scenarios are located #4654

Merged
merged 6 commits into from
Aug 1, 2019

Conversation

jan-cerny
Copy link
Collaborator

Description:

Improve the tests/README.md to document where test scenarios are located and suggests the users to use a bash function to quickly find the test scenarios for a given rule.

Rationale:

Improves the SSGTS documentation.

@jan-cerny jan-cerny added this to the 0.1.46 milestone Jul 25, 2019
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

Very useful function.

Please, also add a oneliner for usage

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
@pep8speaks
Copy link

Hello @jan-cerny! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 27:100: E501 line too long (149 > 99 characters)
Line 65:1: E302 expected 2 blank lines, found 1
Line 76:1: E303 too many blank lines (4)
Line 78:1: W293 blank line contains whitespace
Line 88:1: W293 blank line contains whitespace
Line 101:13: E128 continuation line under-indented for visual indent
Line 103:17: E128 continuation line under-indented for visual indent
Line 123:100: E501 line too long (134 > 99 characters)
Line 132:13: E265 block comment should start with '# '
Line 146:32: W605 invalid escape sequence '\s'
Line 148:34: W605 invalid escape sequence '\s'
Line 166:32: W605 invalid escape sequence '\s'
Line 168:34: W605 invalid escape sequence '\s'
Line 186:32: W605 invalid escape sequence '\s'
Line 188:34: W605 invalid escape sequence '\s'
Line 206:32: W605 invalid escape sequence '\s'
Line 208:34: W605 invalid escape sequence '\s'
Line 226:32: W605 invalid escape sequence '\s'
Line 228:34: W605 invalid escape sequence '\s'
Line 246:32: W605 invalid escape sequence '\s'
Line 248:34: W605 invalid escape sequence '\s'
Line 266:32: W605 invalid escape sequence '\s'
Line 268:34: W605 invalid escape sequence '\s'
Line 275:14: W291 trailing whitespace
Line 286:32: W605 invalid escape sequence '\s'
Line 288:34: W605 invalid escape sequence '\s'
Line 295:14: W291 trailing whitespace
Line 306:32: W605 invalid escape sequence '\s'
Line 308:34: W605 invalid escape sequence '\s'
Line 315:14: W291 trailing whitespace
Line 326:32: W605 invalid escape sequence '\s'
Line 328:34: W605 invalid escape sequence '\s'
Line 346:32: W605 invalid escape sequence '\s'
Line 348:34: W605 invalid escape sequence '\s'
Line 366:32: W605 invalid escape sequence '\s'
Line 368:34: W605 invalid escape sequence '\s'
Line 382:5: E303 too many blank lines (2)
Line 387:32: W605 invalid escape sequence '\s'
Line 389:34: W605 invalid escape sequence '\s'
Line 407:32: W605 invalid escape sequence '\s'
Line 409:34: W605 invalid escape sequence '\s'
Line 426:11: W292 no newline at end of file

Improve the tests/README.md to document where test scenarios are located
and suggests the users to use a bash function to quickly find the test
scenarios for a given rule.
Because function find_tests is expected to return a path I have
changed the name of the function. Also improves wording and adds
an explanation of usage of the aforementioned function.
@jan-cerny
Copy link
Collaborator Author

@yuumasato I have rephrased

@mildas
Copy link
Contributor

mildas commented Jul 30, 2019

@yuumasato Updated

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

I still pitch that a function that prints you the paths for where to find the test scenarios for a rule would be more convenient.

Stretchy goal: a function that creates the directories necessary to test a new rule! (creating these directories is a pain). This will be useful until the tests/data directory is migrated.

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated
rule_id="$1"
ssg_root="/path/to/your/content/git/repository"
tests_dir=$(find $ssg_root/tests/data/ -name *$rule_id*)
[ -n "$tests_dir" ] && cd $tests_dir
Copy link
Member

Choose a reason for hiding this comment

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

If find returns no files, nothing is done, and no message is output.
user: "What happened?"

@mildas
Copy link
Contributor

mildas commented Jul 30, 2019

@yuumasato I changed the function to find test scenarios folder and print path instead of trying cd to folder.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

@mildas Some more changes.

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

@mildas I found a few more issues. Sorry.

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated
test_dir=$(find $ssg_root/tests/data/ -type d -name *$rule_id*)

if [ ! -z "$test_dir" ]; then
printf "Test scenarios for rules their name contains \"$rule_id\" can be found at:\n$test_dir\n"
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is strange.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to rephrase it. But now it's missing the information that the "string" was used as globbing pattern.

tests/README.md Outdated Show resolved Hide resolved
@yuumasato
Copy link
Member

LGTM, thanks @mildas for the updates.

@yuumasato yuumasato self-assigned this Aug 1, 2019
@yuumasato yuumasato merged commit f96fbfa into ComplianceAsCode:master Aug 1, 2019
This pull request was closed.
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.

4 participants