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

src/GlassCat/generate.jl tests #143

Merged
merged 5 commits into from
Apr 27, 2021
Merged

src/GlassCat/generate.jl tests #143

merged 5 commits into from
Apr 27, 2021

Conversation

alfredclwong
Copy link
Collaborator

@alfredclwong alfredclwong commented Apr 23, 2021

The existing GlassCat test sets are a little jumbled.

@testset "Parsing Tests" parses an AGF file TEST_CAT.agf into a Julia Dict cat, but then only checks the first glass' values.
@testset "Module Gen Tests" then checks the generated AGF_TEST_CAT.jl against cat, which creates a dependency on the previous test set.
@testset "Glass Tests" then uses a glass from the TEST_CAT module, which also creates a dependency on the previous test set.

Reorganising these and extending the range of checked values to all 4 glasses catches a couple of odd behaviours that have snuck into GlassCat (mostly due to changes that I've made). I'll fix these before opening the PR for review.

Changes

  • "AGF to dict", "dict to JL" and pure "glass tests" properly organised within their test sets
  • TEST_CAT.agf tests expanded to check all values instead of just the first glass
  • Fix #133, which was caught by the expanded tests
  • Add a TEST GlassType to avoid redefinition of const AGF_GLASSES etc.

@alfredclwong alfredclwong added bug Something isn't working testing Adding missing tests or correcting existing tests labels Apr 23, 2021
@alfredclwong alfredclwong self-assigned this Apr 23, 2021
also fix incorrect TEST_GLASSES test
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2021

Codecov Report

Merging #143 (ffbd1ab) into main (dd60ad1) will decrease coverage by 0.81%.
The diff coverage is 69.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   48.37%   47.55%   -0.82%     
==========================================
  Files          65       64       -1     
  Lines        6659     6614      -45     
==========================================
- Hits         3221     3145      -76     
- Misses       3438     3469      +31     
Impacted Files Coverage Δ
src/GlassCat/GlassTypes.jl 8.86% <61.53%> (+0.45%) ⬆️
src/GlassCat/generate.jl 88.65% <100.00%> (+0.23%) ⬆️
src/Optical/Emitters/Emitters.jl 0.00% <0.00%> (-20.00%) ⬇️
src/Geometry/Primitives/Qtype.jl 75.00% <0.00%> (-8.34%) ⬇️
src/Optical/LensAssembly.jl 48.22% <0.00%> (-7.33%) ⬇️
src/Optical/OpticalInterface.jl 63.23% <0.00%> (-4.30%) ⬇️
src/Geometry/CSG/Intersection.jl 48.57% <0.00%> (-2.14%) ⬇️
src/Geometry/Geometry.jl 0.00% <0.00%> (-1.86%) ⬇️
src/Geometry/Primitives/Curves/BSpline.jl 66.15% <0.00%> (-0.52%) ⬇️
src/Geometry/CSG/Interval.jl 83.20% <0.00%> (-0.37%) ⬇️
... and 4 more

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 dd60ad1...ffbd1ab. Read the comment docs.

@alfredclwong alfredclwong changed the title [WIP] src/GlassCat/generate.jl tests src/GlassCat/generate.jl tests Apr 23, 2021
@alfredclwong alfredclwong marked this pull request as ready for review April 23, 2021 16:10
@alfredclwong alfredclwong enabled auto-merge (squash) April 23, 2021 16:11
src/GlassCat/GlassTypes.jl Show resolved Hide resolved
src/GlassCat/GlassTypes.jl Show resolved Hide resolved
src/GlassCat/generate.jl Outdated Show resolved Hide resolved
test/testsets/GlassCat.jl Show resolved Hide resolved
@BrianGun
Copy link
Contributor

@friggog and @alfredclwong is this PR ready to merge? A conversation doesn't seem resolved.

@alfredclwong
Copy link
Collaborator Author

@BrianGun I haven't yet resolved Charlie's comment on the new glasstype arg. In general I think it's important to have longer, in-depth code review periods and I don't see a need to rush PR merges (unless we're building merge conflicts elsewhere).

@BrianGun
Copy link
Contributor

didn't mean to rush anybody. Just wanted to make sure this wasn't forgotten.

friggog
friggog previously approved these changes Apr 26, 2021
this better reflects the desired usage for the new feature (described
further in the added docstring line)
@alfredclwong alfredclwong merged commit 89b3027 into main Apr 27, 2021
@alfredclwong alfredclwong deleted the fix-glasscat-parsing branch April 27, 2021 08:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working testing Adding missing tests or correcting existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: sourcefile_to_catalog
4 participants