Skip to content

Add code coverage#946

Closed
kingbuzzman wants to merge 73 commits into
FactoryBoy:masterfrom
kingbuzzman:feature/tox-cov
Closed

Add code coverage#946
kingbuzzman wants to merge 73 commits into
FactoryBoy:masterfrom
kingbuzzman:feature/tox-cov

Conversation

@kingbuzzman

@kingbuzzman kingbuzzman commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

When working on the PR "Implements bulk_create for create_batch if available" I found the need to ensure that all the code I wrote was properly tested/covered under tests. Started experimenting and came up with this PR to add coverage when using the CI

@kingbuzzman

kingbuzzman commented Jun 24, 2022

Copy link
Copy Markdown
Contributor Author

@francoisfreitag @rbarrois What do you guys think about this? .. Also it would be nice if you create a key over at codecov / equivalent to upload all the files there and not have to download them to see them (PS you will only be able to download the files for a day) :/

@kingbuzzman kingbuzzman left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LMKWYT

- "3.10"
- "pypy-3.7"
- "pypy-3.8"
- ["3.7", "py37"]

@kingbuzzman kingbuzzman Jun 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adds pretty names.. for the coverage filename. If we move this to codecov / alternative, there would be no need to have this around.

if: matrix.run-coverage == 'yes'
uses: actions/upload-artifact@v3
with:
name: factory_coverage_${{ matrix.python-version[1] }}_${{ matrix.database-type }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would be nice to move this over to a codecov / equivalent and not have to download it..

Comment thread Makefile Outdated
DOC_DIR=docs
EXAMPLES_DIR=examples
SETUP_PY=setup.py
PYTHON_TEST=python \

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved this here to reuse it for coverage support

Comment thread Makefile Outdated
Comment thread setup.cfg
Comment thread setup.cfg Outdated
Comment thread setup.cfg
Comment thread setup.cfg
Comment thread tox.ini
whitelist_externals = make
commands = make test
commands =
!cov: make test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to force anyone to use nocov, this way its more "intelligent", even though there is a bit of "magic".

@kingbuzzman

kingbuzzman commented Jun 24, 2022

Copy link
Copy Markdown
Contributor Author

In the spirit of openness;

There is an issue that has alluded me, this only happens sometimes, and only when using pypy{3.7, 3.8} + sqlalchemy + sqlite + coverage. I have been able to reproduce this locally too, but haven't understood the cause. There seems to be little to no information about this issue.

It's always the same error in the same placefactory_boy/tests/test_alchemy.py::SQLAlchemyGetOrCreateTests. test_simple_call:

Traceback (most recent call last):
    File "/home/runner/work/factory_boy/factory_boy/tests/test_alchemy.py", line 138, in test_simple_call
      obj1 = WithGetOrCreateFieldFactory(foo='foo1')
    File "/home/runner/work/factory_boy/factory_boy/factory/base.py", line 40, in __call__
      return cls.create(**kwargs)
    File "/home/runner/work/factory_boy/factory_boy/factory/base.py", line 528, in create
      return cls._generate(enums.CREATE_STRATEGY, kwargs)
    File "/home/runner/work/factory_boy/factory_boy/factory/alchemy.py", line 59, in _generate
      return super()._generate(strategy, params)
    File "/home/runner/work/factory_boy/factory_boy/factory/base.py", line 465, in _generate
      return step.build()
    File "/home/runner/work/factory_boy/factory_boy/factory/builder.py", line 267, in build
      kwargs=kwargs,
    File "/home/runner/work/factory_boy/factory_boy/factory/base.py", line 317, in instantiate
      return self.factory._create(model, *args, **kwargs)
    File "/home/runner/work/factory_boy/factory_boy/factory/alchemy.py", line 110, in _create
      return cls._get_or_create(model_class, session, args, kwargs)
    File "/home/runner/work/factory_boy/factory_boy/factory/alchemy.py", line 77, in _get_or_create
      obj = cls._save(model_class, session, args, {**key_fields, **kwargs})
    File "/home/runner/work/factory_boy/factory_boy/factory/alchemy.py", line 122, in _save
      session.commit()
    File "<string>", line 2, in commit
    File "/home/runner/work/factory_boy/factory_boy/.tox/pypy37-django32-mongo-alchemy-sqlite-cov/site-packages/sqlalchemy/orm/session.py", line 1451, in commit
      self._transaction.commit(_to_root=self.future)
.... a lot of sqlalchemy functions in between
    File "/home/runner/work/factory_boy/factory_boy/.tox/pypy37-django32-mongo-alchemy-sqlite-cov/site-packages/sqlalchemy/orm/loading.py", line 151, in <listcomp>
      rows = [proc(row) for row in fetch]
    File "/home/runner/work/factory_boy/factory_boy/.tox/pypy37-django32-mongo-alchemy-sqlite-cov/site-packages/sqlalchemy/orm/loading.py", line [89](https://github.com/FactoryBoy/factory_boy/runs/7043440395?check_suite_focus=true#step:6:92)0, in _instance
      dict_ = instance_dict(instance)

example 3.7 failed
example 3.8 failed


Update: Tested it like this

(i=0
while (true); do
    i=$((i+1))
    echo
    echo "running tests $i"'!!'
    echo
    time tox -e pypy37-django32-mongo-alchemy-sqlite-cov
    if [ ! "$?" = "0" ]; then
        printf '\a\n'
        echo "took $i tries"'!!'
        break
    fi
done)

I think i fixed it. It usually failed around 5-10 iterations. I'm by 68 and no crash!! happy dance

Comment thread Makefile Outdated
@rm -rf .coverage htmlcov/

coverage-test:
$(PYTHON_ERROR_ON_WARN) $(COVERAGE_PATH) run -m unittest

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rbarrois friendly bump :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rbarrois friendly bump :)

@kingbuzzman

Copy link
Copy Markdown
Contributor Author

There is no point having this around

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