Adding regression test for LogRecord args issue

[pamelafox]

I started hypothesis testing on LogRecord and discovered an inconsistency that led to a Segmentation Fault in picologging.

Specifically, the C code is trying to emulate:
isinstance(args[0], collections.abc.Mapping)

But it's using PyMapping_Check(firstValue) to do that, and PyMapping_Check isn't equivalent- it returns true for many things, including strings. There is an actual way to check in Python 3.10 that's used by pattern lib:

Py_TYPE(subject)->tp_flags & Py_TPFLAGS_MAPPING;

But that wont work in 3.9. So I added a check that the object is not a string, which at least resolved the seg fault issue that I found. It feels a bit janky though.

I can also check in the hypothesis tests if that's desired.

[codecov-commenter]

Codecov Report

Merging #105 (c7847cb) into main (f500e9e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #105   +/-   ##
=======================================
  Coverage   88.93%   88.93%           
=======================================
  Files          11       11           
  Lines        2069     2069           
=======================================
  Hits         1840     1840           
  Misses        229      229           

Impacted Files	Coverage Δ
src/picologging/logrecord.cxx	85.86% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pamelafox]

Note: failing tests are all due to filename. Doesnt reproduce locally but I'll dig into the code difference.

[tonybaloney]

Considering this field is only accessed in LogRecord_writeMessage and used to call PyUnicode_Format which requires:

/* Apply an argument tuple or dictionary to a format string and return
   the resulting Unicode string. */

We should just check dictionary and tuple.

[pamelafox]

Made the change. For reference, here are the CPython discussions that led to the internal change from dict to collections.abc.mapping:
https://bugs.python.org/issue21172
python/cpython@1b76114

I think it's very unlikely that someone will send in something dict-like, but it could currently happen with stdlib. I could also add a test to ensure that if that does happen, it's not a seg fault and is more of a no-op.

[pamelafox]

@tonybaloney So the hypothesis testing is failing only on Windows 3.7. Your code seems correct from what I read about how to extract a filename, and I don't see any CPython similar code for inspiration. Do you know why the filename would be missing the extension on 3.7/Windows? And does it matter? (Related q: Should I move the hypothesis check outside of the standard flows?)

#ifdef WIN32
const wchar_t* filename_wchar = fs_path.filename().c_str();
const wchar_t* modulename = fs_path.stem().c_str();
entry->filename = PyUnicode_FromWideChar(filename_wchar, wcslen(filename_wchar)),
entry->module = PyUnicode_FromWideChar(modulename, wcslen(modulename));
#else
entry->filename = PyUnicode_FromString(fs_path.filename().c_str());
entry->module = PyUnicode_FromString(fs_path.stem().c_str());
#endif

[tonybaloney]

The test seems flaky, I tried 3.7 locally and it works, then it fails on 3.8 in CI. Perhaps related to the caching of values

[pamelafox]

I've broken the tests into two so that the hypothesis tests won't be run by default -they're in a different folder now, that seems the easiest way to do it so that you don't have to specify keyword arguments for running the standard tests.

For the filename test, I could:

1. comment that line of code with a comment
2. Put filename in its own test and mark it as skip/flaky
3. ..?

[pamelafox]

TODO:

• Put hypothesis workflow on push to main
• Mark filename specific test as flaky

[pamelafox]

@tonybaloney I've made requested changes, please re-review.

[tonybaloney]

Looks good 👍🏻