Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing tests for gap.help #26987

Closed
jdemeyer opened this issue Jan 1, 2019 · 18 comments
Closed

Failing tests for gap.help #26987

jdemeyer opened this issue Jan 1, 2019 · 18 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Jan 1, 2019

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.

CC: @dimpase @embray @slel

Component: doctest coverage

Keywords: GAP

Author: Erik Bray

Branch/Commit: ea643c1

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/26987

@jdemeyer jdemeyer added this to the sage-8.6 milestone Jan 1, 2019
@slel
Copy link
Member

slel commented Jan 1, 2019

Changed keywords from none to GAP

@dimpase
Copy link
Member

dimpase commented Jan 2, 2019

comment:2

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).

@jdemeyer
Copy link
Author

jdemeyer commented Jan 2, 2019

comment:3

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 ...

@fchapoton
Copy link
Contributor

@dimpase
Copy link
Member

dimpase commented Jan 2, 2019

comment:5

Replying to @fchapoton:

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.

@fchapoton
Copy link
Contributor

@dimpase
Copy link
Member

dimpase commented Jan 2, 2019

comment:7

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.

@embray
Copy link
Contributor

embray commented Jan 2, 2019

comment:8

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.

@embray
Copy link
Contributor

embray commented Jan 2, 2019

comment:9

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

@jdemeyer
Copy link
Author

jdemeyer commented Jan 2, 2019

comment:10

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".

@embray
Copy link
Contributor

embray commented Jan 2, 2019

comment:11

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.

@embray
Copy link
Contributor

embray commented Jan 2, 2019

comment:12

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.

@embray
Copy link
Contributor

embray commented Jan 2, 2019

Author: Erik Bray

@embray
Copy link
Contributor

embray commented Jan 2, 2019

Commit: ea643c1

@embray
Copy link
Contributor

embray commented Jan 2, 2019

comment:13

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

@embray
Copy link
Contributor

embray commented Jan 2, 2019

Branch: u/embray/ticket-26987

@vbraun
Copy link
Member

vbraun commented Jan 2, 2019

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jan 3, 2019

Changed branch from u/embray/ticket-26987 to ea643c1

@vbraun vbraun closed this as completed in 7eb4db2 Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants