pcdshub / whatrecord

EPICS IOC meta information tool
https://pcdshub.github.io/whatrecord/
Other
8 stars 6 forks source link

TST: add missing dependency that reveals failing tests from pcds-envs #166

Closed ZLLentz closed 1 year ago

ZLLentz commented 1 year ago

Description

Add blark as a dev/test dependency, in an attempt to reveal a test that fails on pcds-envs but doesn't run at all in the test suite here.

Motivation and Context

In pcds-envs on the weekly build, whatrecord was failing with this message:

_________________________ test_serialize[PlcMetadata] __________________________

cls = <class 'whatrecord.plugins.twincat_pytmc.PlcMetadata'>

    @all_dataclasses
    def test_serialize(cls):
>       instance = try_to_instantiate(cls)

whatrecord/tests/test_serialization.py:151: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
whatrecord/tests/test_serialization.py:123: in try_to_instantiate
    type_hints = apischema.utils.get_type_hints(cls)
/home/runner/miniconda/envs/pcds-test/lib/python3.9/typing.py:1459: in get_type_hints
    value = _eval_type(value, base_globals, localns)
/home/runner/miniconda/envs/pcds-test/lib/python3.9/typing.py:292: in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
/home/runner/miniconda/envs/pcds-test/lib/python3.9/typing.py:554: in _evaluate
    eval(self.__forward_code__, globalns, localns),
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   AttributeError: module 'blark.dependency_store' has no attribute 'ResolvedDependency'

I checked this repo and found that this test never gets run, and I think it's because there's an import error raised here: https://github.com/pcdshub/whatrecord/blob/16a9903c34e072ee7b6a87519f21d5057f5720b0/whatrecord/tests/test_serialization.py#L57 in importing whatrecord.plugins.twincat_pytmc here: https://github.com/pcdshub/whatrecord/blob/16a9903c34e072ee7b6a87519f21d5057f5720b0/whatrecord/plugins/twincat_pytmc.py#L28

Which is because blark isn't installed in the during the test suite.

How Has This Been Tested?

n/a, I just want to see if the test that fails on pcds-envs also fails on the base whatrecord repo it's good to catch these earlier and I think all tests being run is the desired behavior

Where Has This Been Documented?

n/a

ZLLentz commented 1 year ago

OK, yes, this test does fail on whatrecord too if it gets run, not only in the context of pcds-envs, which is a relief because that will make it simpler to track down.

I'm not sure immediately what to do with this PR- I've accomplished my short-term goal of restoring a missing test from the suite.

There are more missing tests to track down. Here's the final line from pcds-envs:

= 1 failed, 428 passed, 2 skipped, 7 xfailed, 10 warnings in 342.16s (0:05:42) =

Here's the final line from whatrecord in this PR:

= 1 failed, 409 passed, 20 skipped, 6 xfailed, 34 warnings in 438.95s (0:07:18) =

Here's the final line from whatrecord before this PR:

===== 403 passed, 20 skipped, 6 xfailed, 34 warnings in 445.79s (0:07:25) ======
klauer commented 1 year ago

There's a bit of work to be done to get whatrecord working with blark again after a big refactor there.

I'm afraid the whatrecord test suite will either have to be added to xfail or blark would have to be pinned to a compatible version (but that'd be less than preferable since the later versions of blark have a lot better source compatibility)

ZLLentz commented 1 year ago

Ok, I'll just leave this sitting here then until we decide what to do. I think it's a major structural problem that specific tests don't get run at all, it gives off a false impression that everything is working fully. It's much better to explicitly skip/xfail them instead of not run them.

klauer commented 1 year ago

I think it's a major structural problem that specific tests don't get run at all, it gives off a false impression that everything is working fully.

Agreed - but such is the nature of these side project monstrosities...

klauer commented 1 year ago

This was merged, but the fix won't be applied until the next tag. Blocker for next tag: #162