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

[V - Non-Rule] Rule-specific testing #51

Merged
merged 6 commits into from
Sep 18, 2023
Merged

[V - Non-Rule] Rule-specific testing #51

merged 6 commits into from
Sep 18, 2023

Conversation

Ghesselink
Copy link
Contributor

There are many files at the moment (78 in a newly developed branch) and there is no good integration (as far as I know) between behave in the command line and gherkin.
This functionality allows to test for specific rules (e.g. 'alb001, 'alb002') as well as categories (e.g. 'alb') just by running 'pytest -sv'

@aothms
Copy link
Collaborator

aothms commented Feb 15, 2023

Instead of hardcoding this I'd rather look for a way to supply this as a command line argument (python test_main.py alb). See for inspiration: https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.7.0/src/ifcopenshell-python/test/test_rules.py#L15 Also see Jakub's earlier PR #39

@Ghesselink
Copy link
Contributor Author

It's now possible with a command line argument (e.g. python test_main.py alb, but also multple arguments as in python test_main.py alb002 alb003)

test_for_rules = any(re.match(r"^[a-zA-Z]{3}\d{3}$", arg) for arg in sys.argv[1:])
test_files = [fn for fn in glob.glob(os.path.join(os.path.dirname(__file__), 
                "files/**/*.ifc"), recursive=True) if len(sys.argv) < 2 or any(x in fn.lower() for x in sys.argv[1:]) or not test_for_rules]

@aothms aothms self-requested a review February 28, 2023 10:29
@@ -11,7 +12,10 @@
sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..'))
from main import run

test_files = glob.glob(os.path.join(os.path.dirname(__file__), "files/**/*.ifc"), recursive=True)
test_for_rules = any(re.match(r"^[a-zA-Z]{3}\d{3}$", arg) for arg in sys.argv[1:])
test_files = [fn for fn in glob.glob(os.path.join(os.path.dirname(__file__),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still find it a bit confusing:

  • test_for_rules: why is this a boolean, why not do filter() instead of any() to filter out all non-rule code argv and use them below
  • the regex is lower or upper case but by doing x in fn.lower() actually an uppercase argument will not work
  • some of the rule codes such as alb will arbitrarily match word fragments such as albino which will be rather confusing. Test files are in separate directories so why not filter only on that directory name instead of entire file path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's modified in 377f4e6

why is this a boolean, why not do filter() instead of any() to filter out all non-rule code argv and use them below

Done.

some of the rule codes such as alb will arbitrarily match word fragments such as albino which will be rather confusing. Test files are in separate directories so why not filter only on that directory name instead of entire file path?

Done.

the regex is lower or upper case but by doing x in fn.lower() actually an uppercase argument will not work

The aim was to also allow for uppercase input, such as 'Alb001' or'ALB001'instead of 'alb001' It should be good now: the regex match will still allow for this uppercase and it later converts into lowercases to match the folder name (which are always in lowercases, such as in 'alb001').

Furthermore, there is a warning added in the case there is someone wants to test a non-existing rule code (e.g. 'alb005'). This could be either a typo or it could mean that there are no test files added yet (or something else even). Nothing too spectecular, but it might help someone writing rules and testing it locally.

@Ghesselink Ghesselink changed the title add functionality to test only for specific rules [V - Non-Rule] Rule-specific testing Apr 21, 2023
@@ -11,8 +12,36 @@
sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..'))
from main import run

test_files = glob.glob(os.path.join(os.path.dirname(__file__), "files/**/*.ifc"), recursive=True)
@pytest.mark.parametrize("filename", test_files)
rule_code_pattern = re.compile(r"^[a-zA-Z]{3}\d{3}$")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have rule_code_pattern and rule_codes duplicated in the function body on line 27.

test/test_main.py Outdated Show resolved Hide resolved
file_pattern = r".*\.ifc(\')?$" #matches ".ifc" and "ifc'"
test_files.extend([s.strip("'") for s in sys.argv if re.match(file_pattern, s)])

if not test_files: # for example, with 'pytest -sv'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of deciding for running all tests at the end on empty test_files, I would decide for this based on an empty [a for a in sys.argv[1:] if not a.startswith('-')]. That way, running with an non-existant rule code will run nothing instead of everything.

Test files for a single file -> 'python3 test_main.py <path>.ifc
Also applies for multiple files -> 'python3 test_main.py <path1>.ifc <path2>.ifc'
Codes and rules can also be combined -> 'python3 test_main.py alb001 <path>.ifc'
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to test that an argument is either a valid rule code or an existing IFC file. In other cases we should do nothing and throw a big error. Otherwise it's easy to think you have ran the tests while you actually did nothing because you misttyped a filename.

@aothms aothms merged commit d68a2b2 into main Sep 18, 2023
2 checks passed
@Ghesselink Ghesselink deleted the limit-rules-tested branch November 1, 2023 10:30
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.

2 participants