Add test workflow with unittest#189
Conversation
aktech
left a comment
aktech
left a comment
There was a problem hiding this comment.
Other than the nitpicks, it looks good to me.
|
@embr This PR seems to me to be ready for review and merge. It just adds a testing workflow. I think fixing the failing tests can come from subsequent work. The test failure is because of an import error. The import refers to code that is "in maintenance mode" and "not available in TFMA OSS". Until the code becomes present in |
|
I'm going to update the branch. If we have failing tests, let's mark them as XFAIL here, then make a PR to remove the missing imports - now that this is officially community maintained, we shouldn't keep tests around for code we will never see here. |
…rom `AttributeError: module 'tensorflow_model_analysis' has no attribute 'EvalConfig'` These are to be addressed in a future PR
smokestacklightnin
force-pushed
the
ci/testing/add-workflow-with-unittest
branch
from
441f05a to
3849ac9
Compare
smokestacklightnin
force-pushed
the
ci/testing/add-workflow-with-unittest
branch
from
662caa2 to
a9e43e6
Compare
smokestacklightnin
force-pushed
the
ci/testing/add-workflow-with-unittest
branch
from
a7b33ab to
a3812b9
Compare
…rkflow-with-unittest
smokestacklightnin
force-pushed
the
ci/testing/add-workflow-with-unittest
branch
from
542f1fa to
47de67f
Compare
.github/workflows/ci-test.yml
Outdated
| - name: Install dependencies | ||
| run: | | ||
| sudo apt update | ||
| sudo apt install -y protobuf-compiler libprotobuf-c-dev |
There was a problem hiding this comment.
I believe these are sufficient for protobufs to be build, but I'm not sure they're both/all strictly necessary.
|
@peytondmurray Tests are all passing or xfailed! Will clean up the PR and ping you for review |
smokestacklightnin
force-pushed
the
ci/testing/add-workflow-with-unittest
branch
from
49b1b85 to
f00949b
Compare
peytondmurray
left a comment
peytondmurray
left a comment
There was a problem hiding this comment.
Just a a couple of comments.
| - name: Run unit tests | ||
| shell: bash | ||
| run: | | ||
| python -m unittest discover -p "*_test.py" |
There was a problem hiding this comment.
How come we don't use pytest here?
There was a problem hiding this comment.
This has to do with pickling errors in pytest that aren't present in unit test
| nightly='>=1.18.0.dev', | ||
| git_master='@git+https://github.com/tensorflow/tfx-bsl@master', | ||
| ), | ||
| 'tf-keras', |
There was a problem hiding this comment.
@embr tests fail without this, but I was wondering if this should be a dependency for the whole package, or only a test dependency.
CC: @peytondmurray
3 participants
Changes in this PR:
unittestfor running unit tests. I tried usingpytest, but there pickling errors when I did that.unittestskips orexpectedFailures to tests with errors or failurestf-kerasas a dependency. Tests fail without this.You can get a list of tests with
skips orexpectedFailures using$ grep -nr "PR 189" tensorflow_model_analysis