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

Version dependent error messages #643

Merged
merged 12 commits into from Aug 30, 2017

Conversation

Projects
None yet
4 participants
@abonie
Contributor

abonie commented Aug 20, 2017

There are multiple discrepancies in error messages for Python 3.5+, which causes 3.5 and 3.6 test suites to fail. This tries to fix them.

@abonie

This comment has been minimized.

Contributor

abonie commented Aug 20, 2017

Anyone knows how to handle situation where for certain test cases there are expected failures in Python 3.4 but unexpected successes for Python 3.5?
@freakboy3742

@lkh01

This comment has been minimized.

Contributor

lkh01 commented Aug 20, 2017

How many of those instances are there? I only see 20 failures in the output from BeeKeeper (no unexpected successes).
Anyway, you might want to make a special decorator that handles those cases. It's only a matter of adding a version check to expectedFailure.

var args
var extra = 0
var lo
var hi

This comment has been minimized.

var extra = 0
var lo
var hi

This comment has been minimized.

var lo
var hi
while (pos < code.co_code.val.length) {

This comment has been minimized.

var hi
while (pos < code.co_code.val.length) {
var opcode_start_pos = pos

This comment has been minimized.

var opcode = code.co_code.val[pos++]
// next opcode has 4-byte argument effectively.
if (opcode === dis.EXTENDED_ARG) {

This comment has been minimized.

// next opcode has 4-byte argument effectively.
if (opcode === dis.EXTENDED_ARG) {
lo = code.co_code.val[pos++]
hi = code.co_code.val[pos++]

This comment has been minimized.

if (opcode in dis.hasconst) {
args = [code.co_consts[intArg]]
} else if (opcode in dis.hasfree) {

This comment has been minimized.

}
} else if (opcode in dis.hasname) {
args = [code.co_names[intArg]]
} else if (opcode in dis.hasjrel) {

This comment has been minimized.

this.push(dict)
return
}
VirtualMachine.prototype.byte_STORE_MAP = function() {
switch (constants.BATAVIA_MAGIC) {
case constants.BATAVIA_MAGIC_35:

This comment has been minimized.

var lenPos = arg % 256
for (var i = 0; i < lenKw; i++) {
var items = this.popn(2)
namedargs[items[0]] = items[1]

This comment has been minimized.

var items = this.popn(2)
namedargs[items[0]] = items[1]
}
if (kwargs) {

This comment has been minimized.

for (let elem of args) {
posargs.push(elem)
}
}

This comment has been minimized.

var retval = func(posargs, namedargs)

This comment has been minimized.

this.push(retval)
var retval = func(posargs, namedargs)
this.push(retval)
}
}
VirtualMachine.prototype.byte_RETURN_VALUE = function() {

This comment has been minimized.

@abonie abonie force-pushed the abonie:version_dependent_error_messages branch 5 times, most recently from 9192c4d to a6caeb3 Aug 20, 2017

@abonie

This comment has been minimized.

Contributor

abonie commented Aug 20, 2017

Thanks lucantrop. There were some unexpected successes, they just did not appear at the bottom.

And it turns out there already was a solution for this problem in not_implemented_versions :)

@abonie abonie force-pushed the abonie:version_dependent_error_messages branch 4 times, most recently from 8675062 to 808bffc Aug 22, 2017

@abonie abonie changed the title from [WIP] Version dependent error messages to Version dependent error messages Aug 22, 2017

@phildini

This comment has been minimized.

phildini commented Aug 23, 2017

This is pretty incredible, and you've got a 🚢 from me.

It's a shame we have to repeat the <3.6 cases so many times. If you felt like condensing that logic at all, that would be fascinating and most likely very helpful.

@abonie

This comment has been minimized.

Contributor

abonie commented Aug 23, 2017

@phildini You mean that CI task for Python 3.4 has to be run each time, even though PR regards Python 3.6?

Well, I am not well-versed with Ops part of DevOps, so one thing that comes to my mind would be parsing commit/PR message by BeeKeeper, looking for some special line like "# test Python 3.6" and changing tasks based on that. This sounds like a mess though, and also note that:

  • Ultimately we would like to have separate branches for different Python versions, and then this problem is easily solved via beekeeper.yml config file.
  • As long as Batavia supports different versions on a single branch, there is a risk, that accidentally someone will mess up how it behaves for Python 3.4 while working on Python 3.5, so running all tests may actually be necessary :(

@abonie abonie force-pushed the abonie:version_dependent_error_messages branch 2 times, most recently from e7cd63c to c07cde4 Aug 23, 2017

@phildini

This comment has been minimized.

phildini commented Aug 23, 2017

Sorry, I should have been clearer!

What I meant was we do

case constants.BATAVIA_MAGIC_34:
case constants.BATAVIA_MAGIC_35a0:
case constants.BATAVIA_MAGIC_35:
case constants.BATAVIA_MAGIC_353:
...
case constants.BATAVIA_MAGIC_36:
...

a whole lot in these changes, which seems like duplicating a lot of code? Also maybe brittle for supporting future versions.

If there were some helper functions to deal with this, it would make me happy, but this whole complaint is more "nit" than "blocker", so feel free to say you don't want to action that right now and we can probably still move forward.

@abonie

This comment has been minimized.

Contributor

abonie commented Aug 23, 2017

@phildini Oh, sure. I have actually already started working on a module that would allow something more like this:

if (version.older('3.6')) {
    ...
} else {
    ...
}

I thought I'd submit it in separate patch though, I am not sure if I can finish it before Monday, since Thursday through Sunday I won't be able to do any work (and it's not super straight forward as there are some caveats with pre/postreleases).

Also this too goes away once we move to one branch per version.

@phildini

This comment has been minimized.

phildini commented Aug 23, 2017

Fair enough! If you get to the point where tests are passing on this branch, I'm happy to merge in and save the optimization for post-merge.

@abonie

This comment has been minimized.

Contributor

abonie commented Aug 23, 2017

Great! BTW I don't know what's going on with CI right now, seems like it crashed on Python lint check..?

@phildini

This comment has been minimized.

phildini commented Aug 23, 2017

I'm not entirely sure what's going on either... could you try rebasing against master and pushing again?

@abonie abonie force-pushed the abonie:version_dependent_error_messages branch from c07cde4 to 603c4c9 Aug 23, 2017

@phildini

This comment has been minimized.

phildini commented Aug 26, 2017

@abonie do you feel this is ready to merge? I'm happy to do so if you're ready.

@@ -26,7 +27,19 @@ function bytes(args, kwargs) {
} else if (args.length === 1) {
var arg = args[0]
if (arg === null) {
throw new exceptions.TypeError.$pyclass("'NoneType' object is not iterable")
switch (constants.BATAVIA_MAGIC) {

This comment has been minimized.

@phildini

phildini Aug 26, 2017

It would be great if we could condense these cases down to a helper function, but I'm fine with that being a cleanup PR later.

@abonie abonie force-pushed the abonie:version_dependent_error_messages branch from 603c4c9 to 82a515f Aug 27, 2017

Mark docstring tests as expected failure
math module's docstring changed in Python 3.5. For now it's ok to mark
respective tests as failing.

@abonie abonie force-pushed the abonie:version_dependent_error_messages branch from 82a515f to 2a6cec1 Aug 27, 2017

@abonie

This comment has been minimized.

Contributor

abonie commented Aug 27, 2017

@phildini I made one last change or rather retracted one of the earlier commits, as it seems that error message in sorted has been changed in Python 3.6.2 (which I had on my machine), but BeeKeeper runs Python 3.6, so that change actually caused a failure. It is definitely ready to be merged. Failing test case in test_str.py in FormatTests seems like more than just an error message discrepancy.

@phildini phildini merged commit c85d032 into pybee:master Aug 30, 2017

5 checks passed

beekeeper:0/beefore:eslint JavaScript lint checks passed.
Details
beekeeper:0/beefore:pycodestyle Python lint checks passed.
Details
beekeeper:1/smoke-test Smoke build (Python 3.4) passed.
Details
beekeeper:2/full-test:py3.5 Python 3.5 tests: non-critical problem found. Click for details.
Details
beekeeper:2/full-test:py3.6 Python 3.6 tests: non-critical problem found. Click for details.
Details

@abonie abonie referenced this pull request Sep 6, 2017

Merged

[WIP] Runtime version checking #654

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment