add Pharo13 support

[estebanlm]

we are about to release Pharo12, which means we need to open Pharo13 :)

[theseion]

Looks good, except for the 404 from the file server.

[estebanlm]

yes, I made the PR before having the images :P
can you re-trigger it ?

[theseion]

Preparing Pharo image...
Unsupported Pharo version 'Pharo64-13'.
Downloading Pharo64-13 vm...
download_file() expects an URL and a target path.
Error: Process completed with exit code 1.

[fniephaus]

Unsupported Pharo version 'Pharo64-13'.

@estebanlm please also update pharo::get_vm_url()

[estebanlm]

I have no idea what is happening now :)

[theseion]

The specification is good now, IMO. I need to look into the failures coming from SmalltalkCI.

[theseion]

Some of the tests fail due to issues with package management, even in Pharo12-stable. That's odd because I had fixed all of that once already. @estebanlm I'd appreciate it if you could help out with those test failures (or @jecisc maybe). From a quick glance it looks like there's some confusion of tags / categories.

[theseion]

Nvm, I figured it out. @fniephaus the last failing test (#testReportFailure) fails due to different ordering of tests, which leads to different output strings. Also, the fixtures appear to be outdated. I don't see a good way to make that test stable to be honest, the fact that it has worked for so long is a miracle...

[theseion]

I spent 30 minutes trying to clean up the stdout reporter tests and then gave up. To properly match the generated output, we'd either have to use a custom runner or implement templating and complex matching. I don't think it's worth the effort. I've removed the test.

[theseion]

Currently, the GemStone builds are failing and I can't debug those. @fniephaus, can you help out?

[fniephaus]

I spent 30 minutes trying to clean up the stdout reporter tests and then gave up. To properly match the generated output, we'd either have to use a custom runner or implement templating and complex matching. I don't think it's worth the effort. I've removed the test.

Could we relax the test, for example, by ensuring every line from the template can be found in the generated output?

[fniephaus]

Currently, the GemStone builds are failing and I can't debug those. @fniephaus, can you help out?

It seems hdiutil is failing for some reason:

UserDefinedError: The command '/usr/bin/hdiutil attach GemStone64Bit3.5.3-arm64.Darwin.dmg' exited with status 1. See the file '/tmp/gsd_stones139996781290505153618979304401482798518.err'

Maybe the .err file would provide us with more details but this really is something @dalehenrich needs to help us with. In the meantime, I think it's ok to mark the failing GemStone builds as allowed to fail.

[theseion]

Could we relax the test, for example, by ensuring every line from the template can be found in the generated output?

I tried that. Unfortunately, the output includes stuff that I'm sure will differ on other platforms. Example:

�[1m�[34m#################################################
�[1m�[34m# Stdout-testReportFailure                      #
�[1m�[34m# 8 Tests with 4 Failures and 1 Errors in 0.02s #
�[1m�[34m#################################################
�[0m
(3 tests passed)
�[1m
SCIExcludedTests
�[0m�[32m ✓�[0m #testDeprecation (0ms)�[0m
�[32m ✓�[0m #testShouldPass (0ms)�[0m
�[32m ✓�[0m #testShouldFail (1ms)�[0m

�[1m�[31m#########################
�[1m�[31m# 5 tests did not pass: #
�[1m�[31m#########################
�[0m�[1m
SCIExcludedTests
�[0m�[1m�[33m ✗ #testThisIsAVeryLongMethodNameThat...playedCorrectlyInATravisLog (7ms)�[0m
�[1mTestFailure: Assertion failed�[0m
SCIExcludedTests(TestAsserter)>>assert:description:resumable:
SCIExcludedTests(TestAsserter)>>assert:description:
SCIExcludedTests(Object)>>assert:
SCIExcludedTests>>testThisIsAVeryLongMethodNameThatProbablyNeedsToBeContractedInOrderToBeDisplayedCorrectlyInATravisLog ...assert: false
SCIExcludedTests(TestCase)>>performTest
�[0m�[1m�[33m ✗ #testShouldPassUnexpectedly (0ms)�[0m
�[1mTestFailure: Test passed unexpectedly�[0m�[1m�[31m ✗ #testError (1ms)�[0m
�[1mError: An error message.�[0m
SCIExcludedTests(Object)>>error:
SCIExcludedTests>>testError ...error: 'An error message.'
SCIExcludedTests(TestCase)>>performTest
�[0m�[1m�[33m ✗ #testFailure (1ms)�[0m
�[1mTestFailure: A failure message.�[0m
SCIExcludedTests(TestAsserter)>>assert:description:resumable:
SCIExcludedTests(TestAsserter)>>assert:description:
SCIExcludedTests(TestAsserter)>>fail:
SCIExcludedTests>>testFailure ...fail: 'A failure message.'
SCIExcludedTests(TestCase)>>performTest
�[0m�[1m�[33m ✗ #testAssertError (1ms)�[0m
�[1mTestFailure: Got 3 instead of 4.�[0m
SCIExcludedTests(TestAsserter)>>assert:description:resumable:
SCIExcludedTests(TestAsserter)>>assert:description:
SCIExcludedTests(TestAsserter)>>assert:equals:
SCIExcludedTests>>testAssertError ...assert: 3 equals: 4
SCIExcludedTests(TestCase)>>performTest
�[0m

�[1m�[31m###########
�[1m�[31m# Summary #
�[1m�[31m###########
�[0m�[1m
SCIExcludedTests
�[0m�[33m ✗ #testThisIsAVeryLongMethodNameThatProbablyNeedsToBeContractedInOrderToBeDisplayedCorrectlyInATravisLog (7ms)�[0m
�[33m ✗ #testShouldPassUnexpectedly (0ms)�[0m
�[31m ✗ #testError (1ms)�[0m
�[33m ✗ #testFailure (1ms)�[0m
�[33m ✗ #testAssertError (1ms)�[0m

�[1m�[31msmalltalkCI Deprecation Warnings�[0m
 - SCIExcludedTests>>testDeprecation (This is just a test)


�[1m�[31m  Executed 8 Tests with 4 Failures and 1 Errors in 0.02s.�[0m

Fixture:

failureFixtureNonTravis
	^ '
�[1m�[34m###############
�[1m�[34m# Stdout-testReportFailure#
�[1m�[34m# 8 Tests with 4 Failures and 1 Errors in s #
�[1m�[34m###############
�[0m
(3 tests passed)
�[1m
SCIExcludedTests
�[0m�[32m ✓�[0m #testDeprecation (ms)�[0m
�[32m ✓�[0m #testShouldFail (ms)�[0m
�[32m ✓�[0m #testShouldPass (ms)�[0m

�[1m�[31m#########################
�[1m�[31m# 5 tests did not pass: #
�[1m�[31m#########################
�[0m�[1m
SCIExcludedTests
�[0m�[1m�[33m ✗ #testAssertError (ms)�[0m
�[0m�[1m�[31m ✗ #testError (ms)�[0m
�[0m�[1m�[33m ✗ #testFailure (ms)�[0m
�[0m�[1m�[33m ✗ #testShouldPassUnexpectedly (ms)�[0m
�[0m�[1m�[33m ✗ #testThisIsAVeryLongMethodNameThat...playedCorrectlyInATravisLog (ms)�[0m

�[1m�[31m###########
�[1m�[31m# Summary #
�[1m�[31m###########
�[0m�[1m
SCIExcludedTests
�[0m�[33m ✗ #testAssertError (ms)�[0m
�[31m ✗ #testError (ms)�[0m
�[33m ✗ #testFailure (ms)�[0m
�[33m ✗ #testShouldPassUnexpectedly (ms)�[0m
�[33m ✗ #testThisIsAVeryLongMethodNameThatProbablyNeedsToBeContractedInOrderToBeDisplayedCorrectlyInATravisLog (ms)�[0m
�[1m�[31msmalltalkCI Deprecation Warnings�[0m
 - SCIExcludedTests>>testDeprecation (This is just a test)


�[1m�[31m  Executed 8 Tests with 4 Failures and 1 Errors in s.�[0m

'

As you can see, the current result includes stack information and other strings that are not part of the fixture.

[estebanlm]

I do not understand. Anything I should do?

[theseion]

No, I need a decision from @fniephaus on how to proceed.

[fniephaus]

As you can see, the current result includes stack information and other strings that are not part of the fixture.

Sure, makes sense. Could the fixture just be an order-independent subset of the expected output? Something like a list of substrings that must be found in the output.

[theseion]

That sounds feasible, yes.

[fniephaus]

Hope it's not too much work, otherwise I'm ok with disabling/removing the test.

[theseion]

I have a working solution. Can you get me the current output for Travis? Or should we just skip the test for Travis?

[fniephaus]

Cool! Feel free to skip the test on Travis. It's not even set up anymore I think.

[theseion]

@fniephaus I give up. I had it and then GemStone got in the way with a failing implementation of String>>#match: (possibly because of WideString) before I discovered that in Pharo 12 sometimes a new line is missing. There's just too much that can go wrong. IMO, at this point the test would be pretty much useless because it has to incorporate too many exceptions. I've removed it now.

[fniephaus]

Ok, thanks for trying anyway! So this is good to go now from your perspective?

[theseion]

Ok, thanks for trying anyway! So this is good to go now from your perspective?

Yes, looks good (with the exception of the GemStone macOS builds).

[theseion]

I'll squash before merging.

[theseion]

The Pharo64-6.1 on windows-2019 job failed to upload coverage results (the job didn't fail). Do you consider that an issue @fniephaus?

Uploading coverage results to Coveralls...
Failed to upload coverage results (HTTP status code #500):
{"message":"Build processing error.","error":true,"url":""}

[fniephaus]

If it fails the build, I guess it's a bug. But nothing that needs to be addressed in this PR. Don't want to delay this any further :)

[fniephaus]

LGTM

[theseion]

Now it looks good. I had to adjust the timeout for Squeak trunk on macOS. For some reason, each stop / start cycle of the images takes multiple minutes.

I can't merge btw, don't have the permission.

[fniephaus]

I can't merge btw, don't have the permission.

That's weird, you do have write permissions which should include permission to merge PRs.

[fniephaus]

Oh, did we just deploy this on a Friday 🤞 😉

[theseion]

IT'S GOING TO BE FINE! 😅

[jecisc]

I wonder if something is missing because we still have this in the CI:

Starting Pharo build...
Unsupported Pharo version 'Pharo64-13'.
Downloading Pharo64-13 image...
download_file() expects an URL and a target path.
Error: Process completed with exit code 1.

[theseion]

Where are you seeing this?

[jecisc]

In most external projects of Pharo. Example:

https://github.com/pharo-spec/NewTools/pull/753/checks

[theseion]

That's because there is no Pharo64-13. Use Pharo64-alpha instead. This is a deliberate choice. With Pharo 12, the image at .../120 was always stale. So while the expectation was that Pharo64-alpha = Pharo64-12, this wasn't true and led to some weird issues. We will add Pharo64-13 before it is released.

[estebanlm]

That's because there is no Pharo64-13. Use Pharo64-alpha instead. This is a deliberate choice. With Pharo 12, the image at .../120 was always stale. So while the expectation was that Pharo64-alpha = Pharo64-12, this wasn't true and led to some weird issues. We will add Pharo64-13 before it is released.

I am sorry but I disagree with that choice. If ../120 (get-files?) was stale there was a problem there, but I do not see why we need to remove the naming for that (which poses some problems in our flow, also).

[theseion]

Ok, I'll add entries for Pharo 13. I'll be counting on your help when stuff breaks ;)