#26987 closed defect (fixed)

Failing tests for gap.help

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.6
Component: doctest coverage Keywords: GAP
Cc: dimpase, embray, slelievre Merged in:
Authors: Erik Bray Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: ea643c1 (Commits) Commit: ea643c1e0b120d925d9f29ba53de76d8e84bf6ff
Dependencies: Stopgaps:

Description

On some patchbots, I see this:

sage -t --long src/sage/interfaces/gap.py
**********************************************************************
File "src/sage/interfaces/gap.py", line 1351, in sage.interfaces.gap.Gap.help
Failed example:
    print(gap.help('SymmetricGroup', pager=False))
Expected:
    <BLANKLINE>
      50.1-... SymmetricGroup
    <BLANKLINE>
      ‣ SymmetricGroup( [filt, ]deg ) ─────────────────────────────────── function
    ...
    <BLANKLINE>
Got:
    <BLANKLINE>
    <BLANKLINE>
      50.1-10 SymmetricGroup
    <BLANKLINE>
      > SymmetricGroup( [filt, ]deg ) ----------------------------------- function
[..many more lines..]

This seems to be a matter of unicode output or not.

Change History (15)

comment:1 Changed 11 months ago by slelievre

  • Cc dimpase embray slelievre added
  • Keywords GAP added

comment:2 follow-up: Changed 11 months ago by dimpase

Please provide a link to the full log. It is not clear whether "many more lines" mean more errors or not.

For this particular one I am inclined just to add more ...

I don't think Sage must produce the same output for different locales, or whatever the real reason for this discrepancy is (perhaps just a patchbot bug).

comment:3 in reply to: ↑ 2 Changed 11 months ago by jdemeyer

Replying to dimpase:

Please provide a link to the full log.

I don't know where it was, but the problem is obvious.

It is not clear whether "many more lines" mean more errors or not.

Not errors, since anything after that wrong line is caught by the already-existing ...

comment:5 in reply to: ↑ 4 Changed 11 months ago by dimpase

Replying to chapoton:

full log at the bottom of

https://patchbot.sagemath.org/log/26985/Ubuntu/18.04/x86_64/3.13.0-123-generic/44e979ad077a/2018-12-31%2020:58:39?short

This is with 8.6.rc0 - it would be more interesting to see rc1, where more GAP 4.10 is merged.

comment:7 Changed 11 months ago by dimpase

is it possible to find out what exactly happens there - is it print() unable to output UTF-8, or GAP outputting its help in ASCII, not UTF-8? E.g. I have

sage: type(gap.help('SymmetricGroup', pager=False))
<type 'unicode'>

and it prints OK, too.

comment:8 Changed 11 months ago by embray

It is most likely a question of the system's locale. This has happened before with other random patchbots that have things like LC_CTYPE=C and such. It should instead set a UTF-8 locale. GAP will not use unicode if it does not detect a unicode locale.

I think the patchbot itself should either set LC_CTYPE, or perhaps even the Sage doctest runner. While it can be interesting to catch locale-related failures, for the sake of builds it's probably best to ensure a consistent environment, and only manipulate locales for specific regression tests for known locale-related bugs.

comment:9 follow-up: Changed 11 months ago by embray

  • Priority changed from blocker to minor

I don't think this is a blocker since it's a system environment issue on the affected patchbots.

comment:10 in reply to: ↑ 9 Changed 11 months ago by jdemeyer

  • Priority changed from minor to blocker

Replying to embray:

I don't think this is a blocker since it's a system environment issue on the affected patchbots.

So you consider a patchbot environment broken if it doesn't support UTF-8? We generally try to support UTF-8 and non-UTF-8 locales in Sage doctests.

And if you're really convinced that it's a patchbot problem, this bug should be closed as "workforme", not downgraded to "minor".

comment:11 follow-up: Changed 11 months ago by embray

It's obviously not a blocker if it's just a trivial formatting issue in some test that can be fixed by setting the locale on your system.

I do think something should be done about this though so closing it as "worksforme" would be equally reactionary. Whether it's just adding ... in those tests (fine by me), or as I suggested above fixing the locale either in the tests, the doctest runner, or the patchbot (I don't have an opinion about which would be most appropriate), or whether we should actually force a locale on GAP when initializing libgap.

comment:12 in reply to: ↑ 11 Changed 11 months ago by embray

Replying to embray:

whether we should actually force a locale on GAP when initializing libgap.

To clarify, in this case the test in question is only affected by the pexpect interface. So the output from GAP in that case is going to depend on the environment passed when starting gap.

comment:13 Changed 11 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-26987
  • Commit set to ea643c1e0b120d925d9f29ba53de76d8e84bf6ff
  • Status changed from new to needs_review

I think should suffice for now; leave other questions 'til later.


New commits:

1c845fcallow setting additional environment variables when starting a Gap interface instance
dc9b9edforce a UTF-8 locale on GAP in these tests
ea643c1trivial cleanup

comment:14 Changed 11 months ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:15 Changed 11 months ago by vbraun

  • Branch changed from u/embray/ticket-26987 to ea643c1e0b120d925d9f29ba53de76d8e84bf6ff
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.