Stix21master

[khdesai]

No description provided.

[chisholm]

The errors seem to be the same in every case. It has to do with checking error messages, e.g.

assert 'is not a valid argument for a Start/Stop Qualifier' in str(excinfo)

The test error is always:

AssertionError: assert 'is not a valid argument for a Start/Stop Qualifier' in '<ExceptionInfo ValueError tblen=2>'
+  where '<ExceptionInfo ValueError tblen=2>' = str(<ExceptionInfo ValueError tblen=2>)

I think the problem is that the code str(excinfo) isn't producing what the test author expected. The author expected it to produce something which contains the error message associated with the ValueError. But pytest.raises actually produces a wrapper around the actual exception, and evidently the format of its str() isn't considered a stable public interface and something to be relied upon. I think to get the real error message, you should get the real exception, which is available as excinfo.value [1].

I think this may have only surfaced now because of a recent change to pytest. There is a changelog entry for 5.0.0 which mentions this [2]. This apparently came about from a github issue [3]. Looks to have been intentional, to keep people from getting confused about what the pytest.raises() contextmanager produces: if it's not the actual exception, its str() shouldn't make people think it's the actual exception. The "<ExceptionInfo ...>" format makes it clear that what you have is an ExceptionInfo wrapper.

To fix this, I think we need to change all instances of str(excinfo) to str(excinfo.value). Another option is to delete those checks, since as I've said, I don't think we should be checking human-targeted messages like this anyway.

What do you think?

1. https://docs.pytest.org/en/latest/assert.html#assertraises
2. https://docs.pytest.org/en/latest/changelog.html
3. str() on the pytest.raises context variable doesn't behave same as normal exception catch pytest-dev/pytest#5412

[clenk]

I'm fine with removing those string comparison checks.

[codecov-io]

Codecov Report

Merging #275 into stix2.1 will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           stix2.1     #275      +/-   ##
===========================================
+ Coverage    98.64%   98.66%   +0.01%     
===========================================
  Files          120      122       +2     
  Lines        12921    13060     +139     
===========================================
+ Hits         12746    12885     +139     
  Misses         175      175

Impacted Files	Coverage Δ
stix2/test/v21/test_core.py	100% <ø> (ø)	⬆️
stix2/test/v21/test_datastore_filters.py	100% <ø> (ø)	⬆️
stix2/v21/__init__.py	100% <ø> (ø)	⬆️
stix2/test/v21/test_bundle.py	100% <ø> (ø)	⬆️
stix2/test/v21/test_versioning.py	100% <ø> (ø)	⬆️
stix2/test/v21/test_workbench.py	72.72% <ø> (ø)	⬆️
stix2/test/v21/test_indicator.py	100% <100%> (ø)	⬆️
stix2/test/v21/test_infrastructure.py	100% <100%> (ø)
stix2/test/v21/constants.py	100% <100%> (ø)	⬆️
stix2/test/v20/test_datastore_filesystem.py	99.19% <100%> (-0.01%)	⬇️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 953a91b...b464a9c. Read the comment docs.

[emmanvg]

@khdesai has removed the checks for the tests. The rest of the changes look good.