-
Notifications
You must be signed in to change notification settings - Fork 32
(closes #3250 #279) Complete code coverage #3258
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3258 +/- ##
==========================================
+ Coverage 99.91% 99.96% +0.04%
==========================================
Files 376 376
Lines 53529 53482 -47
==========================================
- Hits 53484 53461 -23
+ Misses 45 21 -24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@arporter This is ready for review. It brings the codecov misses from 45 to 27 and adds no-cover pragmas to the remaining (maybe you know how to do better in the remaining?). I also added a more strict reporting for deleted lines (indirect changes). |
arporter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for tackling this Sergi.
There are a couple of cases where I think we can add tests rather than no-cover pragmas. Also, you've removed some validation code from ACCEnterDataTrans and I don't understand why.
| # mixed-precision kernel. | ||
| alg_precision = api_config.precision_map[alg_arg.precision] | ||
| if alg_precision != actual_precision: | ||
| if alg_precision != actual_precision: # pragma: no-cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding the following test to lfric_kern_test.py will cover this line. I'll let you write a suitable docstring :-)
def test_validate_kernel_code_arg_against_alg_arg(monkeypatch):
'''
'''
psy, invoke = get_invoke("26.8_mixed_precision_args.f90", TEST_API,
name="invoke_0", dist_mem=False)
sched = invoke.schedule
# Second kernel has args of precision r_solver.
kernel = sched.walk(LFRicKern, stop_type=LFRicKern)[1]
read_access = ArgumentInterface(ArgumentInterface.Access.READ)
lfric_real_scalar_symbol = LFRicTypes("LFRicRealScalarDataSymbol")(
"scalar", interface=read_access)
with pytest.raises(GenerationError) as err:
kernel._validate_kernel_code_arg(lfric_real_scalar_symbol,
lfric_real_scalar_symbol,
kernel.args[0])
assert ("Precision (4 bytes) of algorithm-layer argument "
"'scalar_r_solver' does not match that (8 bytes) of the "
"corresponding kernel subroutine argument 'scalar' for kernel "
"'mixed_code'" in str(err.value))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| # if the kernel requires a grid-property argument. | ||
| return None | ||
|
|
||
| @property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you've also update lfric.py (thanks) so I've added #279 to the issues this PR will close (as I've grepped and those are the only mentions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
src/psyclone/psyGen.py
Outdated
| return [self._field] | ||
|
|
||
| def check_vector_halos_differ(self, node): | ||
| def check_vector_halos_differ(self, node): # pragma: no-cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
psyGen_test does have some tests of this method so it can't all be uncovered? In fact, I've grepped and it seems this method is completely unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I delete it and there was no issues
| # no cover: start | ||
| try: | ||
| # pylint disable=import-outside-toplevel | ||
| from termcolor import colored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we monkeypatch the packages that appear to be available from termcolor? I had a google and found https://stackoverflow.com/questions/8658043/how-to-mock-an-import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think it's quite important to get coverage for this because if we somehow broke it it would break a lot of stuff!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted this for now, as I see we have the same problem with graphviz and I have not yet figure out how to solve it
src/psyclone/tests/gocean1p0_test.py
Outdated
|
|
||
|
|
||
| def test_invalid_field_accesses_for_iteration_space(): | ||
| ''' Tests that we when retrieving the iteration_space_arg we validate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/we when/when/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| ''' UnsupportedType is abstract so sub-class it for this test ''' | ||
| def __str__(self): | ||
| return "OtherType" | ||
| ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know you could do that!
|
|
||
|
|
||
| def test_empty_program_handler(parser): | ||
| '''Test that an empty program handler still returns a FileContainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the program handler still returns a FileContainer for an empty program" perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's better
| # extract async. Default to False. | ||
| async_queue = options.get('async_queue', False) | ||
|
|
||
| # check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come you've removed this? I see we still support the specification of an async_queue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we checked that all directives in the same schedule had the same async_queue? Wouldn't that mean that we cannot do ACCEnterData asynchronously at all?
src/psyclone/transformations.py
Outdated
| try: | ||
| kernels = node.get_callees() | ||
| except SymbolError as err: | ||
| except SymbolError as err: # pragma: no-cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be easy to cover as we can monkeypatch node.get_callees() with a function that just raises SymbolError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
@arporter This is ready for another look. |
No description provided.