Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

Test rework #23

Merged
merged 10 commits into from
Mar 19, 2021
Merged

Test rework #23

merged 10 commits into from
Mar 19, 2021

Conversation

alfredclwong
Copy link
Collaborator

@alfredclwong alfredclwong commented Mar 18, 2021

These changes are just suggestions - I'm putting them into a PR for review/discussion.

@BrianGun I remember you saying that you don't use our custom Precompile/Sysimage code anymore - if this is the case, should we clean these files? Similarly, I don't see usages for Miscellaneous.jl (aka Junk) or Spectra.jl. Let me know if they're still here for a reason and I'll revert the specific commits.

Specifying a separate test/Project.toml helps us to lighten the core dependency list.

The big change: I think Benchmarks, Diagnostics and TestData naturally belong in test/. Some docs/examples use TestData, but otherwise they're largely separate from the core functionality and they take up a good chunk of compile time. We can just include($path_to_TestData) for the few exceptions, as I've done here. What do you guys think?

@alfredclwong alfredclwong requested a review from BrianGun March 18, 2021 11:54
@codecov-io
Copy link

Codecov Report

Merging #23 (7dc34ec) into main (11509d9) will increase coverage by 4.84%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   44.58%   49.42%   +4.84%     
==========================================
  Files          59       53       -6     
  Lines        7131     6116    -1015     
==========================================
- Hits         3179     3023     -156     
+ Misses       3952     3093     -859     
Impacted Files Coverage Δ
src/OpticSim.jl 50.00% <ø> (ø)
src/Optical/Paraxial.jl 64.00% <0.00%> (+1.50%) ⬆️

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 11509d9...7dc34ec. Read the comment docs.

@BrianGun
Copy link
Contributor

BrianGun commented Mar 18, 2021

@alfredclwong
moving the files is okay.

Deleting Spectra.jl is okay.

Deleting Miscellaneous.jl is okay. There is just one example in there that is useful. I sent it to you and Ran in email.

Don't remove the precompile file. It executes some visualization code during precompilation that makes time to first render faster. Makes a noticeable difference. The build task in VSCode doesn't do this. I have some ideas about what we could do that might make this more seamless. I'll create a new issue.

The reason I stopped using it was because Julia Computing support hypothesized that some corrupt type information generated during the precompilation caused by the precompile file was causing an obscure bug in our code. But we haven't had any trouble like that for a while so it was probably a transient thing.

If you modify your commit to undo the rm of the precompile file I'll ok the pull request.

@BrianGun BrianGun merged commit 61481f8 into main Mar 19, 2021
@BrianGun BrianGun deleted the test-rework branch March 19, 2021 05:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants