Opened 13 months ago

Closed 12 months ago

Last modified 12 months ago

#26017 closed defect (fixed)

Improve regression test on search_doc

Reported by: embray Owned by:
Priority: major Milestone: sage-8.4
Component: misc Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: 691cca7 (Commits) Commit:
Dependencies: Stopgaps:

Description

This test started failing for me, due to there being more than 2000 matches for "tree" in my docs (even when considering whole word matches). The rewritten test demonstrates, I believe, that the failure is not in the code itself, but rather that my docs directory just started having a larger number of matches than the hard-coded threshold. I'm not sure why that is, and it's possible I need to do a make doc-clean.

But nevertheless this test can still be rewritten to not depend on some arbitrary thresholds.

(Incidentally, I also noticed that this full text search over all the docs is very, very slow. Doesn't Sphinx generate some indices we could be taking advantage of...?)

Change History (19)

comment:1 Changed 13 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 13 months ago by git

  • Commit changed from cad76698fa8c360586ad8c66dbc8fcd0fb9b9f08 to 48ba5ae36815afb34473ef5f28ee100baa2a7f91

Branch pushed to git repo; I updated commit sha1. New commits:

48ba5aewhitespace cleanup

comment:3 Changed 13 months ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to positive_review

comment:4 Changed 13 months ago by embray

FWIW running a make doc-clean and rebuilding the docs in the source tree where I was getting this failure also fixed the failure of the original test, as I suspected it might. Nevertheless I think this rewrite makes more sense.

comment:5 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:6 Changed 13 months ago by embray

Dare I ask what with??

comment:7 Changed 13 months ago by embray

If you're going to write "merge conflict" you could at least produce the diff as a mere hint of what it might conflict with.

Last edited 13 months ago by embray (previous) (diff)

comment:8 Changed 13 months ago by embray

  • Status changed from needs_work to needs_info

comment:9 Changed 13 months ago by saraedum

I guess that's just Volker's workflow. He merges things into his private "alpha" branch and reports merge conflicts that you won't see until the next beta. I think this kind of workflow is fine but the communication could be a bit more clear about this (as I have already been on quite a few tickets where this caused a bit of confusion.) Something like

The Release Manager could not merge the changes on this ticket because there were merge conflicts. These conflicts might not be visible to you until the next beta of Sage has been released. Please merge the develop branch into this ticket's branch once the next beta version of Sage has been released.

vbraun: What do you think?

comment:10 Changed 13 months ago by embray

I've already brought this up before and suggested that the "private" branch be more visible. The response to this has usually been along the lines of "it's not hidden--you can see it right here". But that's extremely non-obvious. I might know that (now) but it's a pretty confusing and unwelcoming response to new users. I have suggested that it be an easily locatable, well-known branch name on git.sagemath.org (I would prefer an integration branch for the next release or something like that) along with links to that branch so that I can see what's going on there.

The response to that has generally been along the lines of "we can't do that because it would be confusing if people base stuff off that branch". But I don't buy that. Even if someone did, so what? Just say that it doesn't merge and why, and that they should rebase on "develop" or something. I think in practice that would be pretty rare. Much less rare than telling that their ticket has a merge conflict--when by appearances it doesn't--and then not explaining what the conflict is. And then worse, setting the ticket back to "needs work", where it's possible it will be forgotten and decay further.

comment:11 Changed 13 months ago by chapoton

I tried to propose something (in May) :

https://github.com/sagemath/git-trac-command/pull/30

comment:12 Changed 12 months ago by jhpalmieri

See #26102 for a slight speed-up to search_doc.

comment:13 Changed 12 months ago by git

  • Commit changed from 48ba5ae36815afb34473ef5f28ee100baa2a7f91 to 40c91b3e077c8f623f34e59295bd8bacdf424fc4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

40c91b3rewrite this test to not depend on relatively arbitrary bounds

comment:14 Changed 12 months ago by embray

  • Status changed from needs_info to positive_review

Rebased, I guess. Thanks for the info.

comment:15 Changed 12 months ago by vbraun

  • Status changed from positive_review to needs_work
**********************************************************************
File "src/sage/misc/sagedoc.py", line 1179, in sage.misc.sagedoc.?
Failed example:
    len(L) < N  # long time
Exception raised:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 650, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1061, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.sagedoc.?[1]>", line 1, in <module>
        len(L) < N  # long time
    NameError: name 'L' is not defined
**********************************************************************
File "src/sage/misc/sagedoc.py", line 1183, in sage.misc.sagedoc.?
Failed example:
    all(tree_re.search(l) for l in L)  # long time
Exception raised:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 650, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1061, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.sagedoc.?[4]>", line 1, in <module>
        all(tree_re.search(l) for l in L)  # long time
    NameError: name 'L' is not defined
**********************************************************************

comment:16 Changed 12 months ago by jhpalmieri

  • Branch changed from u/embray/misc/search_doc/doctest to u/jhpalmieri/misc/search_doc/doctest

comment:17 Changed 12 months ago by jhpalmieri

  • Commit changed from 40c91b3e077c8f623f34e59295bd8bacdf424fc4 to 691cca71282ee91e92d5d07bd49deaa447f03d10
  • Status changed from needs_work to positive_review

Fixed the optional tags on the doctests.


New commits:

691cca7trac 26017: fix optional tags on doctests

comment:18 Changed 12 months ago by vbraun

  • Branch changed from u/jhpalmieri/misc/search_doc/doctest to 691cca71282ee91e92d5d07bd49deaa447f03d10
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 12 months ago by jdemeyer

  • Commit 691cca71282ee91e92d5d07bd49deaa447f03d10 deleted

Breakage: #26184

Note: See TracTickets for help on using tickets.