Skip to content

testing: increase installer coverage#15237

Merged
alalazo merged 34 commits intospack:developfrom
tldahlgren:tests/increase-installer-coverage
Mar 12, 2020
Merged

testing: increase installer coverage#15237
alalazo merged 34 commits intospack:developfrom
tldahlgren:tests/increase-installer-coverage

Conversation

@tldahlgren
Copy link
Copy Markdown
Contributor

@tldahlgren tldahlgren commented Feb 26, 2020 •
edited
Loading

This PR is dedicated to increasing coverage of the new installer.py module.

@tldahlgren tldahlgren added WIP tests General test capability(ies) labels Feb 26, 2020
@tldahlgren tldahlgren self-assigned this Feb 26, 2020
@tldahlgren tldahlgren mentioned this pull request Feb 27, 2020
Merged
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Feb 29, 2020

Note d5d602f is just to determine if coverage combine is needed to cover the forked build_process in installer._install_task, which appears to have no coverage despite the fork call being made.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 2, 2020

Reset status to see if that will re-run the tests.

@tldahlgren tldahlgren closed this Mar 2, 2020
@tldahlgren tldahlgren reopened this Mar 2, 2020
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 2, 2020

Interesting. Looks like explicitly adding coverage combine to run-unit-tests causes a fatal error:

No data to combine
The command "share/spack/qa/run-$TEST_SUITE-tests" exited with 1.

So reverted.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 2, 2020

FYI. As of commit 58a641e, installer.py has 90.61% coverage (i.e., everything but the forked bulid_process.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 3, 2020 •
edited
Loading

Very strange. Coverage for eeceb04 at https://codecov.io/gh/spack/spack shows counts on comment and lines -- see lines starting 668 -- and no coverage for tests that are passing (e.g., lines ~1101 exercised by test_install_task_stop_iter).

@tldahlgren tldahlgren mentioned this pull request Mar 4, 2020
Merged
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 5, 2020

Ugh! The test that covers the one line being highlighted as uncovered in codecov/project was moved to #15339 .

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Mar 5, 2020

@tldahlgren See #15354

@tldahlgren tldahlgren requested a review from becker33 March 10, 2020 01:45
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 10, 2020 •
edited
Loading

@tldahlgren See #15354

That was great! It revealed most of build_process was already being covered.

alalazo
alalazo reviewed Mar 11, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Leaving a few minor comments, tests look good to me. Can you rebase the PR?

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 11, 2020

@alalazo I addressed your feedback and we're getting ci test and coverage results but the builds are complaining about fetching archives. Should I try closing and re-opening?

@tldahlgren tldahlgren closed this Mar 12, 2020
@tldahlgren tldahlgren reopened this Mar 12, 2020
@tldahlgren tldahlgren closed this Mar 12, 2020
@tldahlgren tldahlgren reopened this Mar 12, 2020
@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 12, 2020 •
edited
Loading

@alalazo I addressed your feedback and we're getting ci test and coverage results but the builds are complaining about fetching archives. Should I try closing and re-opening?

Well, they were there -- patch at 100%, project at -.16% -- until I opened and closed the PR twice in a row. Maybe it will reset them from CI failed once the CI tests finish. T.T

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Mar 12, 2020

@tldahlgren The PR looks good to me. There were issues with our CI due to (I think) some change in Github apt repositories that were solved in #15458. If you can do a final update to this PR I'll merge it as soon as it goes green.

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 12, 2020

@tldahlgren The PR looks good to me. There were issues with our CI due to (I think) some change in Github apt repositories that were solved in #15458. If you can do a final update to this PR I'll merge it as soon as it goes green.

Thanks @alalazo

@tldahlgren
Copy link
Copy Markdown
Contributor Author

tldahlgren commented Mar 12, 2020

@alalazo They are passing now.

@alalazo alalazo merged commit f2a13af into spack:develop Mar 12, 2020
tgamblin pushed a commit that referenced this pull request Mar 20, 2020
@tldahlgren @tgamblin
testing: increase installer coverage (#15237)
1c8f792
likask pushed a commit to likask/spack that referenced this pull request Apr 7, 2020
Merge commit 'e2b1737a42c9c0c796671f9dd0c39f623e4c91c0' into develop_…
5473531 
…upstream_master

* commit 'e2b1737a42c9c0c796671f9dd0c39f623e4c91c0': (1343 commits)
  update CHANGELOG.md for 0.14.1
  version bump: 0.14.1
  multiprocessing: allow Spack to run uninterrupted in background (spack#14682)
  Cray bugfix: TERM missing while reading default target (spack#15381)
  Upstreams: don't write metadata directory to upstream DB (spack#15526)
  Creating versions from urls doesn't modify class attributes (spack#15452)
  bugfix: fix install_missing_compilers option bug from v0.14.0 (spack#15416)
  bugfix: installer.py shouldn't be executable (spack#15386)
  Add function replace_prefix_nullterm for use on mach-o rpaths. (spack#15347)
  ArchSpec: fix semantics of satisfies when not concrete and strict is true (spack#15319)
  suite-sparse: fix installation for v5.X (spack#15326)
  testing:  increase installer coverage (spack#15237)
  bugfix: resolve undefined source_pkg_dir failure (spack#15339)
  Bugfix: resolve StopIteration message attribute failure (spack#15341)
  Recover coverage from subprocesses during unit tests (spack#15354)
  Correct pytest.raises matches to match (spack#15346)
  bugfix:  Add dependents when initializing spec from yaml (spack#15220)
  Uniquify suffixes added to module names (spack#14920)
  bugfix: ensure proper dependency handling for package-only installs (spack#15197)
  Fix for being able to 'spack load' packages that have been renamed. (spack#14348)
  ...

# Conflicts:
#	.travis.yml
#	lib/spack/spack/modules/common.py
#	var/spack/repos/builtin/packages/mofem-cephas/package.py
#	var/spack/repos/builtin/packages/mofem-fracture-module/package.py
#	var/spack/repos/builtin/packages/mofem-users-modules/package.py
#	var/spack/repos/builtin/packages/python/package.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalazo alalazo alalazo approved these changes

@becker33 becker33 Awaiting requested review from becker33

Assignees

@tldahlgren tldahlgren

Labels

impact-high tests General test capability(ies)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tldahlgren @alalazo @tgamblin