Use at-testset for tests #294

mortenpi · 2016-09-28T20:47:10Z

A pretty big bunch of changes attempting to organize tests in a more readable manner. Addresses #154. I see that #288 doesn't have any tests yet, so I'm hopefully not conflicting with it. Motivation comes from #282 -- I wanted to make a few small changes to examples/ for it.

A summary of the changes:

Move to @testset.
Allow building examples/ separately (useful for testing changes to HTML writer etc.).
No toplevel stuff (modules, types) within a @testset macro, so we need to define modules/types outside it. Hence navnode.jl depends on faketypes.jl and examples/tests.jl (and due to an include, runtests.jl) depends on examples/make.jl.
Style change: indent module contents (except if it is the only thing in the file) for readability.
Consider pages/ to be part of the examples/ (but also use them + the modules Mod and AutoDocs, defined now in examples/make.jl, in runtests.jl for unit tests).

Things are quite interlinked here and so this ended up being this one big commit at the moment. I am considering splitting it up into several commits to make it easier to review (most importantly to make sure that we're not losing or making useless any of the existing tests). However a discussion on what the final result should look like would be useful before I get to that.

TODO:

Fix 0.4 tests.
A couple of tests failed due to the module structure changes (marked with TODOs).
Add more explanatory comments.

MichaelHatherly · 2016-09-29T05:26:39Z

Thanks for doing this! I'll take a closer look once I've gotten the LaTeX stuff out of the way.

MichaelHatherly

Looks fine to me. The indentation for inner modules is definitely a good idea, would be good to add a line to the style guide page mentioning it.

mortenpi

So, it became quite a few commits. However, due to the nature of changes, i.e. copy-pasting blocks of code, I think it is a good idea to keep them as separate as possible.

I tried to avoid making any changes to the tests themselves. The few things I had to change I pointed out in the comments (and hopefully didn't forget any).

mortenpi · 2016-10-02T22:25:14Z

docs/src/man/contributing.md

@@ -21,6 +21,7 @@ Follow the style of the surrounding text when making changes. When adding new fe
 ### Julia

  * 4-space indentation;
+  * modules spanning entire files should not be indented, but modules that have surrounding code should;


Added a note here about the modules. Thoughts on the phrasing very welcome.

Wording sounds good to me.

mortenpi · 2016-10-02T22:26:28Z

test/docsystem.jl

+
+import Documenter: Documenter, DocSystem
+
+const alias_of_getdocs = DocSystem.getdocs # NOTE: won't get docstrings if in a @testset


I had to move this here, since the test on line 66 started failing (d_4 had 0 docstrings).

mortenpi · 2016-10-02T22:28:11Z

test/formats/latex.jl

+info("Building Documenter's docs with LaTeX.")
+const Documenter_root = normpath(joinpath(dirname(@__FILE__), "..", "..", "docs"))
+doc = makedocs(
+    debug = true,


Added a debug flag here.

mortenpi · 2016-10-02T22:29:25Z

test/utilities.jl

+        a = Documenter.Utilities.filterdocs(doc, Set{Module}())
+        b = Documenter.Utilities.filterdocs(doc, Set{Module}([UnitTests]))
+        c = Documenter.Utilities.filterdocs(doc, Set{Module}([Base]))
+        d = Documenter.Utilities.filterdocs(doc, Set{Module}([UtilitiesTests]))


Had to change the module name here.

mortenpi · 2016-10-02T22:43:09Z

Appveyor failures due to path handling, I'll have to fix those. I was wondering why we have the joinpaths here and there 😆.

Best practice wise though -- should we construct all paths with joinpath or just when we actually use them to compare paths etc.? It doesn't seem necessary for includes, for example.

It is nice to see the testsets in action though:

Test Summary: | Pass  Fail  Error  Total
  Documenter  |  278     2      1    281
  Utilities   |   17                  17
  NavNode     |   19                  19
  DocSystem   |   34                  34
  DOM         |  132                 132
  MDFlatten   |   20                  20
  Examples    |   35     2      1     38
  Markdown    |   34     2      1     37
  HTML        |    1                   1
  Markdown    |   20                  20
  LaTeX       |    1                   1

MichaelHatherly · 2016-10-03T06:10:07Z

Best practice wise though -- should we construct all paths with joinpath or just when we actually use them to compare paths etc.? It doesn't seem necessary for includes, for example.

I'd say just always use joinpath when creating paths.

It is nice to see the testsets in action though:

Awesome!

Move the code of setting up the modules and building the examples docs under examples/ to test/examples/make.jl. This allows the examples/ to be built without calling the whole runtests.jl file. The pages/* are considered to be part of the examples/. The Mod and AutoDocs modules are now indented to help with readability. Also, the output from the examples/ builds will now be stored under test/examples/builds/<format>, instead of test/examples/build-<format>.

Moves the tests of the examples/ build to examples/tests.jl. The tests can be run separately as well.

Also properly indent the test code and add some logic to check if make.jl has been loaded properly.

And otherwise reorganize the file a bit. Fixes #154.

mortenpi · 2016-10-04T19:22:28Z

I fixed the paths in examples/tests.jl that were causing issues. So, other than the things below, it should be good to go.

However, I do feel that for includes (e.g. in runtests.jl) it should be sufficient to use the forward slash. Julia handles these paths correctly on Windows and it seems more readable:
include("examples/make.jl")
vs
include(joinpath(dirname(@__FILE__), "examples", "make.jl")).

runtests.jl is slightly inconsistent at the moment though -- the old ones use joinpath, but the new ones I added don't. I should probably make it consistent though, so I would like to drop the joinpaths.

I also took the liberty of dropping branch restrictions from appveyor.yml -- I couldn't build custom branches on my fork and I don't see a particular need for them. If we want to restrict the branches built in the JuliaDocs repo we could do that in the AppVeyor instead. However, if you think we should keep it there, I can drop the commit.

MichaelHatherly · 2016-10-05T16:33:09Z

Yeah, paths in include can stay as is without joinpath. All changes look fine to me. I'm currently travelling so feel free to merge once you're happy with this.

Julia already handles cross-platform matters and always includes files relative to the current file, so they're not necessary and dropping them makes the code more readable.

mortenpi · 2016-10-05T17:27:19Z

Thanks for reviewing! I removed the few remaining include(joinpath(...)) cases for consistency. Will merge once the tests pass.

MichaelHatherly · 2016-10-07T15:37:50Z

Thanks for sorting this one out @mortenpi!

mortenpi added Status: In Progress Type: Maintenance Type: Tests labels Sep 28, 2016

MichaelHatherly reviewed Sep 29, 2016

View reviewed changes

Add a style note on module indentation

585a9e8

mortenpi commented Oct 2, 2016

View reviewed changes

mortenpi added 12 commits October 4, 2016 20:56

Move examples/ tests to a separate file

172e1c7

Moves the tests of the examples/ build to examples/tests.jl. The tests can be run separately as well.

Move missing docs tests to a separate file

051af35

Indent submodules of Tests in runtests.jl

9ed346d

Use testsets for examples/tests.jl

6cbca40

Also properly indent the test code and add some logic to check if make.jl has been loaded properly.

Use testsets for navnode.jl

eb524dd

Move DOM tests to separate file and use testsets

988f463

Move DocSystem tests to a sep. file, use testsets

5189bb7

Move Utilities tests to a sep. file, use testsets

f1b03df

Move output tests to separate files, use testsets

0f31e39

Use testsets in runtests.jl

4da069b

And otherwise reorganize the file a bit. Fixes #154.

Remove branch restrictions from appveyor.yml

ef5e3ec

mortenpi added the Status: Review Needed label Oct 4, 2016

MichaelHatherly removed the Status: Review Needed label Oct 5, 2016

Don't use joinpath in test includes

cdc1e14

Julia already handles cross-platform matters and always includes files relative to the current file, so they're not necessary and dropping them makes the code more readable.

mortenpi added Status: Pending and removed Status: In Progress labels Oct 5, 2016

mortenpi merged commit f77cd32 into JuliaDocs:master Oct 6, 2016

mortenpi deleted the reorganize-tests branch October 6, 2016 07:27

mortenpi added Status: Completed and removed Status: Pending labels Oct 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use at-testset for tests #294

Use at-testset for tests #294

mortenpi commented Sep 28, 2016 •

edited

Loading

MichaelHatherly commented Sep 29, 2016

MichaelHatherly left a comment

mortenpi left a comment

mortenpi Oct 2, 2016

MichaelHatherly Oct 3, 2016

mortenpi Oct 2, 2016

mortenpi Oct 2, 2016

mortenpi Oct 2, 2016

mortenpi commented Oct 2, 2016

MichaelHatherly commented Oct 3, 2016

mortenpi commented Oct 4, 2016

MichaelHatherly commented Oct 5, 2016

mortenpi commented Oct 5, 2016

MichaelHatherly commented Oct 7, 2016


		import Documenter: Documenter, DocSystem

		const alias_of_getdocs = DocSystem.getdocs # NOTE: won't get docstrings if in a @testset

Use at-testset for tests #294

Use at-testset for tests #294

Conversation

mortenpi commented Sep 28, 2016 • edited Loading

A summary of the changes:

TODO:

MichaelHatherly commented Sep 29, 2016

MichaelHatherly left a comment

Choose a reason for hiding this comment

mortenpi left a comment

Choose a reason for hiding this comment

mortenpi Oct 2, 2016

Choose a reason for hiding this comment

MichaelHatherly Oct 3, 2016

Choose a reason for hiding this comment

mortenpi Oct 2, 2016

Choose a reason for hiding this comment

mortenpi Oct 2, 2016

Choose a reason for hiding this comment

mortenpi Oct 2, 2016

Choose a reason for hiding this comment

mortenpi commented Oct 2, 2016

MichaelHatherly commented Oct 3, 2016

mortenpi commented Oct 4, 2016

MichaelHatherly commented Oct 5, 2016

mortenpi commented Oct 5, 2016

MichaelHatherly commented Oct 7, 2016

mortenpi commented Sep 28, 2016 •

edited

Loading