search

KarthikRIyer / swiftplot

Swift library for Data Visualization :bar_chart:
Apache License 2.0
400 stars 39 forks source link

conf: Update AGG version to 2.6 #93

Closed KarthikRIyer closed 4 years ago

KarthikRIyer commented 4 years ago

Hmm. Why are these tests failing. I checked a few of these files on my system and couldn't find any difference. Also the existing master branch has many test fails on my system.

karwa commented 4 years ago

There are no visual differences (checked with Github Desktop, copying them over the reference files and checking the "difference" view shows no differences), so it seems like AGG is doing something slightly differently.

The Base64 test shows that the new images are 4 bytes longer than the previous ones. Maybe a bug was fixed in AGG.

Not sure about master - CI verifies that the images are bit-for-bit reproducible.

karwa commented 4 years ago

Oh, but one thing that is concerning - the new histogram tests (testHistogramStackedStepLineJoins, testHistogramMultiStackedStep, testHistogramMultiStacked) do not verify their output. They pass, but they should fail like the others do.

WilliamHYZhang commented 4 years ago

Can we try just overwriting the Reference folder with the locally generated output to fix?

karwa commented 4 years ago

Yes, that's our standard procedure for updating the reference images.

First, you run the tests and see if anything actually changed. If it did, you compare the actual output with the reference output and figure out what exactly changed (you can use a special visual-diffing tool, or just open the folders side-by-side in tabs in the macOS Finder, open a quicklook window, and use CMD+[ and CMD+] to flick between them). If you're sure the changes are intentional, replace the reference image with the new image, check that the tests now pass, and people will discuss in the PR if it's good to make the change or not.

KarthikRIyer commented 4 years ago

I don't think swift works properly with WSL. Yesterday I'd been running the tests on WSL and was getting test failures even with the master branch. So today I tried it with Ubuntu. The master tests passed and I was getting the correct fails with agg updated. Now that I've updated the reference images, some of them which weren't failing before are failing now (the most recent tests @karwa added for origin shifting). I don't know what's wrong. Could someone else see if it's working on their system and update it if I merge this PR?

karwa commented 4 years ago

Ah... it turns out, I forgot to add those tests to LinuxMain.swift, so you'll need to regenerate it with swift test --generate-linuxmain. Both Github's CI and I use macs, so we didn't catch this. But those tests didn't run on your machine, so everything appeared to pass and you didn't get new reference images for them.

Perhaps we should add something to CI to catch this. We could add a step which regenerates linuxmain and checks that there's no file changes, or even just add a linux bot to be extra-sure there are no platform-specific differences.

KarthikRIyer commented 4 years ago

@karwa I'll look into regenerating the linux main in CI. In the meantime could you please update the tests from your side?

karwa commented 4 years ago

@KarthikRIyer Sure: https://github.com/KarthikRIyer/swiftplot/pull/97