Skip to content
This repository has been archived by the owner. It is now read-only.

Version dependent error messages #643

Merged
merged 12 commits into from Aug 30, 2017

Conversation

@abonie
Copy link
Contributor

@abonie 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
Copy link
Contributor Author

@abonie 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

@hgluka
Copy link
Contributor

@hgluka hgluka 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

var extra = 0
var lo
var hi

var lo
var hi

while (pos < code.co_code.val.length) {

var hi

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

var opcode = code.co_code.val[pos++]

// next opcode has 4-byte argument effectively.
if (opcode === dis.EXTENDED_ARG) {

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


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

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

this.push(dict)
return
}

VirtualMachine.prototype.byte_STORE_MAP = function() {
switch (constants.BATAVIA_MAGIC) {
case constants.BATAVIA_MAGIC_35:

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

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

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


var retval = func(posargs, namedargs)

this.push(retval)
var retval = func(posargs, namedargs)
this.push(retval)
}
}

VirtualMachine.prototype.byte_RETURN_VALUE = function() {

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

@abonie 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 version_dependent_error_messages branch 4 times, most recently from 8675062 to 808bffc Aug 22, 2017
@abonie abonie changed the title [WIP] Version dependent error messages Version dependent error messages Aug 22, 2017
@phildini
Copy link
Member

@phildini 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
Copy link
Contributor Author

@abonie 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 version_dependent_error_messages branch 2 times, most recently from e7cd63c to c07cde4 Aug 23, 2017
@phildini
Copy link
Member

@phildini 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
Copy link
Contributor Author

@abonie 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
Copy link
Member

@phildini 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
Copy link
Contributor Author

@abonie 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
Copy link
Member

@phildini 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 version_dependent_error_messages branch from c07cde4 to 603c4c9 Aug 23, 2017
@phildini
Copy link
Member

@phildini 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) {
Copy link
Member

@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 version_dependent_error_messages branch from 603c4c9 to 82a515f Aug 27, 2017
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 version_dependent_error_messages branch from 82a515f to 2a6cec1 Aug 27, 2017
@abonie
Copy link
Contributor Author

@abonie 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 beeware:master Aug 30, 2017
5 checks passed
@abonie abonie mentioned this pull request Sep 6, 2017
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants