Improved cw-2.py #571

Bubbler-4 · 2018-02-10T07:16:00Z

This PR introduces many improvements based on the discussions at #569 and my own Kumite.

Testing Functions

Make testing functions non-blocking
Change the expression inside assert_not_equals so that it can prevent operator injection hacks
Provide pass_, fail, and assert_approx_equals with customizable precision (pass is a Python keyword)
Replace hardcoded prints with display

Describe and It

Build the decorator version of describe and it, so the text fixture may look like this (and the decorator itself runs the code, so it doesn't need a separate runner function):

@describe('describe-text')
def describe1():
    @it('it-text', before=f1, after=f2)
    def it1():
        # some test function
    @it('it-text')
    def it2():
        # some test function

Properly close describe and it blocks in the test output
Print the running times of describe and it blocks
Provide before and after for describe and it

Timeout Utility

Apply a timeout limit on any block of code (the parameter is in seconds, and floats are accepted):

@timeout(0.01)
def some_block():
    do_something()
    test_something()

This is guaranteed to generate a failed test when the block timeouts

The Unicode Error Trap

Print using "\uXXXX" and "\UXXXXXXXX" literal style for tagged messages
Print using "&#XXXX;" HTML escape style for simple logs
This addresses cw-2.py throws UnicodeEncodeError #508 and Python does not support special characters such as ♠♥♣♦ #545, but not Encoding Issue in Python3 Runner #558 (which, I guess, could be solvable by providing the source charset on the first line, which in turn requires some edits in the runner code, not cw-2.py)

* Make testing functions non-blocking * Change the expression inside `assert_not_equals` so that it can prevent operator injection hacks * Provide `pass_`, `fail`, and `assert_approx_equals` * Replace hardcoded `print`s with `display`

@it

* Build the decorator version of `describe` and `it`, so the text fixture may look like this (and the decorator itself runs the code, so it doesn't need a separate runner function): ```python @Describe('describe-text') def describe1(): @it('it-text', before=f1, after=f2) def it1(): # some test function @it('it-text') def it2(): # some test function ``` * Properly close `describe` and `it` blocks in the test output * Print the running times of `describe` and `it` blocks * Provide `before` and `after` for `describe` and `it`

* Print using `"\uXXXX"` and `"\UXXXXXXXX"` literal style for tagged messages * Print using `"&#XXXX;"` HTML escape style for simple logs

Bubbler-4 · 2018-02-10T12:52:27Z

@kazk Would it be OK to add runner-side changes upon this, if this PR isn't going to be merged soon?

kazk · 2018-02-10T19:06:29Z

@Bubbler-4 Thanks! Looks good to me so far :)
Can you elaborate on "runner-side" changes? If you mean the fix to avoid concatenation, then I'm interested.
Edit: Maybe you meant the fix required for #558? I'm interested in that as well.

kazk · 2018-02-10T19:14:39Z

test/runners/python_spec.js

@@ -225,3 +225,102 @@ describe('Output format commands', function() {
    });
  }
 });
+
+describe('Fixed and new features', function() {


Can you also add the test for the Unicode fix (#508, #545)?

could be solvable by providing the source charset on the first line, which in turn requires some edits in the runner code

Do you mean adding # coding: utf-8?

Yes. But since we're going for multiple files approach, we'll need it in each file (at least preloaded, solution, and fixture).

If we go multiple files, can users just add them when needed?
This is currently impossible because we concatenate them and the magic comment is only valid if it's in the first or the second line.

Since we'll want to prepend something to the code when needed (just like in your unittest fix), I'd just add it to every file I create. At least it won't break existing things (because utf-8 is a superset of ascii).

Bubbler-4 · 2018-02-10T22:41:29Z

For the runner changes, I'm thinking about these (blatantly copied from my kumite's comment section):

Add to the first line # -*- coding: utf-8 -*- so we can use Unicode literals without \u escape
Write each source file into .py files using writeFileSync and then import them in the runner code
Remove sanitizeStdErr since its existence isn't relevant anymore, and giving full stack trace is much better choice
Probably we can also write runner.py and run with python runner.py instead of python -c ... which is ugly af

So it covers both the discussion here and #558, but it'll be really breaking changes.

kazk · 2018-02-10T23:02:45Z

We can't have any breaking changes for this repository. Which of those are breaking changes?

# -*- coding: utf-8 -*- and writing files looks fine to me. And removing sanitizeStdErr is technically a breaking change, but shouldn't have effects so I think we should be fine.

We don't even need to import cw-2.py to runner.py; what if we do so in fixture.py?

Yeah, I dislike how some languages runner do these. Also that reload and path manipulation shouldn't be necessary if we set PYTHON_PATH appropriately, right?

I think moving them to fixture.py is fine. My main concern with concatenation is line numbers in error messages users see are not matching their solution.

This reverts commit 46a4ae4.

Bubbler-4 · 2018-02-10T23:46:13Z

Huh, the Unicode fix isn't working properly within the test environment (it was only tested in non-TDD kumite). I'll see if I can fix it properly (if nothing helps, we'd need to resort to print_ instead of print).

kazk · 2018-02-11T00:00:11Z

Python 2.7 is printing '\uac00\n' when expected to include '가'
Python 3.4, 3.6 is throwing:

UnicodeEncodeError: 'ascii' codec can't encode character '\uac00' in position 0: ordinal not in range(128)

kazk · 2018-02-11T00:18:16Z

test/runners/python_spec.js

+        languageVersion: v,
+        testFramework: 'cw-2',
+        code: 'a = 1',
+        fixture: 'print("u\\uac00")',


Did you mean fixture: 'print(u"\\uac00")',? "u vs u". Not sure if that makes any difference.

For Python 2.7, fixing this should make the test pass.

print(u"\uac00") #=> 가 print("u\uac00") #=> u\uac00

kazk · 2018-02-11T00:35:52Z

frameworks/python/cw-2.py


 class AssertException(Exception):
    pass


+'''Fix the dreaded Unicode Error Trap'''
+def print(*args, **kwargs):


This print doesn't seem to work when given non-string, e.g., print(1) throws

TypeError: 'int' object is not iterable

in Python 2.7 and

TypeError: print() argument after * must be a sequence, not map

in Python 3

kazk · 2018-02-11T00:37:13Z

frameworks/python/cw-2.py

+    def _replace(c):
+        if ord(c) >= 128: return u'&#{};'.format(ord(c))
+        return c
+    def _escape(s): return ''.join(_replace(c) for c in s)


In Python 2, the above error is from this _escape.

kazk

print needs to behave the same way as the builtin one
The Unicode fix needs to be fixed or removed
Maybe provide a convenience function instead of replacing print()?

kazk · 2018-02-11T00:45:27Z

test/runners/python_spec.js

+        languageVersion: v,
+        testFramework: 'cw-2',
+        code: 'a = 1',
+        fixture: 'print("u\\uac00")',


For Python 2.7, fixing this should make the test pass.

print(u"\uac00") #=> 가 print("u\uac00") #=> u\uac00

Bubbler-4 · 2018-02-11T01:35:41Z

I'm out today, so changes won't be made until tomorrow.

kazk · 2018-02-12T01:17:43Z

Don't worry, take your time. I just wanted to leave some feedback because I don't know if I have the time to review during the week.

kazk · 2018-02-13T06:17:02Z

@Bubbler-4
For Python 3 throwing error, setting environment variable PYTHONIOENCODING=UTF-8 might resolve the issue and it might not be necessary to escape them.

Setting environment variable can be done by changing
https://github.com/Codewars/codewars-runner-cli/blob/dbf943d62c8d6ed451d27ac1817c0f13b1de558c/lib/runners/python.js#L37

to

    run({
      name: pythonCmd(opts),
      args: ['-c', code],
      options: {
        env: Object.assign({}, process.env, {PYTHONIOENCODING: 'UTF-8'})
      }
    });

kazk · 2018-02-13T06:21:53Z

frameworks/python/cw-2.py

-    def _escape(s): return ''.join(_replace(c) for c in s)
-
+    def _escape(s):
+        if isinstance(s, six.string_types):


Apparently six.string_types doesn't include unicode, so replace this with six.text_type?

https://pythonhosted.org/six/#six.string_types

Bubbler-4 · 2018-02-13T06:35:47Z

Finally solved it, without using environment variables:

Fall back to providing uni_print instead of overwriting print (I forgot Py2 runner doesn't have from __future__ import print_function at the beginning)
Force cast to six.text_type, then apply some regex for better output in Py2 (nested object in Py2 is printed using __str__ which makes unicode literals into dumb u'\uxxxx' style)

kazk · 2018-02-13T06:40:24Z

test/runners/python_spec.js

@@ -329,9 +329,12 @@ describe('Fixed and new features', function() {
        languageVersion: v,
        testFramework: 'cw-2',
        code: 'a = 1',
-        fixture: 'print(1, "2", u"\\uac00")',
+        fixture: 'test.uni_print(1, "a", u"\\uac00", [314159, "b", u"\\uac01"])',


For Python 2, this is printing:

1 a 가 [314159, 'b', u'각']

https://travis-ci.org/Codewars/codewars-runner-cli/jobs/340822289#L1401

Should I remove the u flag in front of the literal? I think I'll leave it there, because str and unicode are two different things in Py2.

Ok, I didn't realize that was intentional. So it'll be rendered like this:

Yes, I tested the output using my kumite on CW, but I don't know how it'll be displayed on Qualified.

kazk · 2018-02-13T07:45:23Z

Looks good to me! I'll try to do some testing against existing contents during this week.
Great job on keeping it backwards compatible.
I'm little concerned that the print("<COMPLETEDIN::>") workaround some users have been doing will confuse the test output parser though. Hopefully there's not too many and power users can help with finding and removing them.

And thanks for looking into the Unicode issue and uni_print!

Bubbler-4 · 2018-02-13T07:56:38Z

The workaround should work just fine because, if describe or it is used old-style, it does not emit <COMPLETEDIN::> at all:

Glad you like it 😄

# Conflicts: # test/runners/python_spec.js

kazk · 2018-02-22T19:19:36Z

@Bubbler-4 Sorry for the delay. I didn't have the time to test this last week.
Just a quick update. I've deployed this to the new runner environment that I've been testing against mirrored CW traffic and haven't found compatibility issues so far 👍

kazk · 2018-02-24T23:03:53Z

I found the root cause of the Unicode issue: https://bugs.python.org/issue19846
The image we currently use on Codewars has no locale set (LC_CTYPE="POSIX"), forcing ASCII :(

The new runner uses the official Docker image with (LANG C.UTF-8) and we can just print Unicode without issues.
For Python 2 image, I've also set PYTHONIOENCODING=UTF-8 because sys.stdout.encoding was None (setting this will do .encode("utf-8") implicitly if I'm understanding correctly).
Overriding the encoding of stdin/stdout/stderr might cause issues for daily use, but should be fine for our use case.

Now we have print(u'\u2713') in Python 2.7/3.4/3.6 output ✓ 🎉

Voileexperiments · 2018-02-27T09:28:39Z

When will this be employed to the main CW site (or preview site)? I miss new changes :(

kazk · 2018-02-27T18:49:42Z

I haven't found anything major (looking at the number of successful Python completions) so this should be safe to merge.
@jhoffner what do you think?

If we do, try not to use test.uni_print. It's no longer necessary on the new runner so I'll remove that eventually. Current timeline to deploy the new runner for CW is by the end of next week.

kazk · 2018-03-14T20:35:33Z

@Bubbler-4
preview.codewars.com uses the new runner now if you want to try using this on Kumite. Just be careful not to publish a kata with it.
Thanks for your work!

Bubbler-4 · 2018-03-15T08:32:45Z

I just made an example kumite, and the new framework works perfectly 👍

Just one thing is, it looks like the runner overhead has increased by ~500ms. It shouldn't be too much of a concern except for a few katas that are insanely tight on the bound, but maybe you can consider increasing the time limit to 12500ms.

kazk · 2018-03-15T17:58:57Z

Thanks for trying it out. I realized that describes can be nested too 👍

The issue with printing Unicode should be resolved as well. I've also avoided concatenation so the error message should be more clear. But I might need to revert it because I noticed some kata uses test methods in preloaded section.

Yeah, the wall time (time displayed at the top) won't be the same because the time includes some extra steps. The actual test execution time (time inside test results) shouldn't be that different.

I might make some adjustments later if this is a major problem, but overall the performance should be better and more stable for most of the languages. I'm not too worried because the old runner have inconsistent performance and the wall time can vary more than 500ms.

Bubbler-4 · 2018-03-15T23:40:33Z

But I might need to revert it because I noticed some kata uses test methods in preloaded section.

How about prepending cw-2.py import to setup.py as well? It's not exposed to the solver, so it doesn't matter if its line numbers are correct or not. Also, that part is the single greatest improvement for kata solvers, so we can't give it up so easily.

kazk · 2018-03-15T23:47:26Z

Yeah, concatenation is terrible for UX. I'll prepend test = Test = __import__('cw-2') to setup.py.

kazk · 2018-03-22T18:23:22Z

@Bubbler-4
avoiding concatenation breaks ~70 kata. Do you think you can help fixing them?
See https://github.com/Codewars/codewars.com/wiki/List-of-Affected-Kata

Python is executed like the following:

Writing Files

setup.py (preloaded, if given)

test = Test = __import__('cw-2')
# preloaded

solution.py

from setup import * # if preloaded is given
# solution

main.py

from solution import *
test = Test = __import__('cw-2')
# tests

Run

python main.py

Many of the affected kata can be fixed by avoiding the use of private variables (_foo) declared in other scopes. Some needs more work, for example, preloaded code referencing solution.

Bubbler-4 · 2018-03-25T06:08:59Z

@kazk
Blind4Basics made a suggestion to improve expect_error so that it can target a specific exception class. Here is my kumite that implements the idea, along with some test cases. Is it possible to add this to the new runner?

kazk · 2018-03-25T08:39:28Z

I've added it with slight modification. expect_error(message, function, exception=Exception).

I'll deploy it maybe tomorrow along with other languages that I need to fix.

kazk · 2018-03-26T00:39:59Z

@Bubbler-4 expect_error is on preview now, see this fork.

kazk · 2018-04-12T22:09:04Z

I'm going to merge this so @Bubbler-4 gets some credit for it. The actual source that was derived from this will be open sourced later.

Blind4Basics · 2018-07-22T15:32:40Z

Hi guys,

I might have found a way to efficiently forbid the import of modules in python (I doubt it will be bullet proof, I'm not enough in the deep structure of python to know. Though, it seems at least not easy to crack). That could be a nice addition to the test framework, I believe, in the following manner:

function/data stored in the file of the framework
a tuple of strings available for the user, each string being the name of a module to forbid (some comments in addition to explain the functioning of it)
a bit of code to execute just after the preloaded section has been imported and before the import of the file of the user.

I'd send to both of you (@Bubbler-4, @kazk) the code by MP on gitter (I'd rather like that it's not visible for now), so that you can tell me if you think it's worth of the addition.

Cheers,
B4B

Bubbler-4 added 7 commits February 10, 2018 07:18

Fix and improve testing functions

d069c93

* Make testing functions non-blocking * Change the expression inside `assert_not_equals` so that it can prevent operator injection hacks * Provide `pass_`, `fail`, and `assert_approx_equals` * Replace hardcoded `print`s with `display`

Fix the Unicode Error Trap

6ba8e4b

* Print using `"\uXXXX"` and `"\UXXXXXXXX"` literal style for tagged messages * Print using `"&#XXXX;"` HTML escape style for simple logs

Add python cw-2 spec on fixed and new features

70fc49a

Copy python/cw-2.py to python3/cw-2.py

74b15bb

Update the example fixtures to new style

04a6c34

Minor edits

6cdc3ee

kazk suggested changes Feb 10, 2018

View reviewed changes

Add tests for unicode output

8735255

kazk approved these changes Feb 10, 2018

View reviewed changes

Bubbler-4 added 3 commits February 11, 2018 08:38

Add tests for unicode output

46a4ae4

Revert "Add tests for unicode output"

2e7c17f

This reverts commit 46a4ae4.

Re-add tests for unicode output

66180f1

kazk reviewed Feb 11, 2018

View reviewed changes

kazk suggested changes Feb 11, 2018

View reviewed changes

Try to fix Unicode Fix

bae62f8

kazk reviewed Feb 13, 2018

View reviewed changes

2nd try

c3c3f7d

kazk reviewed Feb 13, 2018

View reviewed changes

kazk approved these changes Feb 13, 2018

View reviewed changes

Merge branch 'master' into fix/python-cw-2

9a9e10c

# Conflicts: # test/runners/python_spec.js

kazk mentioned this pull request Apr 7, 2018

Encoding Issue in Python3 Runner #558

Closed

kazk merged commit d1c1d8b into codewars:master Apr 12, 2018

Improved cw-2.py #571

Improved cw-2.py #571

Uh oh!

Conversation

Bubbler-4 commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing Functions

Describe and It

Timeout Utility

The Unicode Error Trap

Uh oh!

Bubbler-4 commented Feb 10, 2018

Uh oh!

kazk commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubbler-4 commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kazk commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bubbler-4 commented Feb 10, 2018

Uh oh!

kazk commented Feb 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubbler-4 commented Feb 11, 2018

Uh oh!

kazk commented Feb 12, 2018

Uh oh!

kazk commented Feb 13, 2018

Uh oh!

kazk Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bubbler-4 commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazk commented Feb 13, 2018

Uh oh!

Bubbler-4 commented Feb 13, 2018

Uh oh!

kazk commented Feb 22, 2018

Uh oh!

kazk commented Feb 24, 2018

Uh oh!

Voileexperiments commented Feb 27, 2018

Uh oh!

Bubbler-4 commented Feb 10, 2018 •

edited

Loading

kazk commented Feb 10, 2018 •

edited

Loading

Bubbler-4 commented Feb 10, 2018 •

edited

Loading

kazk commented Feb 10, 2018 •

edited

Loading

kazk Feb 13, 2018 •

edited

Loading

Bubbler-4 commented Feb 13, 2018 •

edited

Loading

Bubbler-4 commented Mar 15, 2018 •

edited

Loading