[tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

Stefan Westerfeld-2

Soundfont audio tests are currently failing on bionic (18.04). As suggested here:

https://mail.gnome.org/archives/beast/2018-May/msg00001.html

I propose a fix that

  • regenerates .ref files to match the 18.04 output
  • uses a lower threshold for old fluidsynth versions as to not break the tests on 16.04,...

I was able to find the corresponding fluidsynth changelog entry, which is (fluidsynth 1.1.7):

which suggests that the new behaviour is caused by a bugfix.


You can view, comment on, or merge this pull request online at:

  https://github.com/tim-janik/beast/pull/42

Commit Summary

  • BUILD: determine soundfont test threshold from fluidsynth version
  • AF-TESTS: fix soundfont test for newer (>= 1.1.7) fluidsynth versions

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","potentialAction":{"@type":"ViewAction","target":"https://github.com/tim-janik/beast/pull/42","url":"https://github.com/tim-janik/beast/pull/42","name":"View Pull Request"},"description":"View this Pull Request on GitHub","publisher":{"@type":"Organization","name":"GitHub","url":"https://github.com"}}</script> <script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Fix af-tests/soundfont-test on bionic (#42)"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/42"}}}</script> <script type="application/ld+json">{ "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "37567f93-e2a7-4e2a-ad37-a9160fc62647", "title": "Fix af-tests/soundfont-test on bionic (#42)", "sections": [ { "text": "", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ ] }, { "title": "Commit Summary", "facts": [ { "name": "0db1ca6", "value": "BUILD: determine soundfont test threshold from fluidsynth version" }, { "name": "017557a", "value": "AF-TESTS: fix soundfont test for newer (\u003e= 1.1.7) fluidsynth versions" } ] }, { "title": "File Changes", "facts": [ { "name": "Modified", "value": "[af-tests/Makefile.sub](https://github.com/tim-janik/beast/pull/42/files#diff-0) (2 changes)" }, { "name": "Modified", "value": "[af-tests/soundfont-test.ref](https://github.com/tim-janik/beast/pull/42/files#diff-1) (556 changes)" }, { "name": "Modified", "value": "[configure.ac](https://github.com/tim-janik/beast/pull/42/files#diff-2) (10 changes)" } ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 42,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 42\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42.patch" } ], "@type": "OpenUri", "name": "View patch" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42.diff" } ], "@type": "OpenUri", "name": "View diff" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 341834315\n}" } ], "themeColor": "26292E" }</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

Stefan Westerfeld-2

Hi Stefan, thanks for the PR.

Overall it looks good, there's one thing that can be improved though.
I'd like to keep all thresholds in one place, in the Makefile.sub. I see two ways to accomplish that:

a) Change configure.ac's FLUID_THRESHOLD to a FLUID_RELAXED_THRESHOLD boolean and pick the threshold inside the Makefile based on the boolean.

b) Change configure.ac's FLUID_THRESHOLD to FLUID_VERSION and pick a threshold inside the Makefile based on FLUID_VERSION>=1.1.7.

I guess (a) suffices for now, but (b) would be more future proof in case we have to adjust the threshold ever again. Come to think of it, we could run into future problems with other libraries as well. Scattering the threshold and version checks across Makefile.sub and configure.ac for each should be avoided. I.e. I'd merge either (a) or (b), but have a slight preference for (b) in terms of setting a precedent for future patches.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","potentialAction":{"@type":"ViewAction","target":"https://github.com/tim-janik/beast/pull/42#issuecomment-395023503","url":"https://github.com/tim-janik/beast/pull/42#issuecomment-395023503","name":"View Pull Request"},"description":"View this Pull Request on GitHub","publisher":{"@type":"Organization","name":"GitHub","url":"https://github.com"}}</script> <script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tim-janik in #42: Hi Stefan, thanks for the PR.\r\n\r\nOverall it looks good, there's one thing that can be improved though.\r\nI'd like to keep all thresholds in one place, in the Makefile.sub. I see two ways to accomplish that:\r\n\r\na) Change configure.ac's FLUID_THRESHOLD to a FLUID_RELAXED_THRESHOLD boolean and pick the threshold inside the Makefile based on the boolean.\r\n\r\nb) Change configure.ac's FLUID_THRESHOLD to FLUID_VERSION and pick a threshold inside the Makefile based on FLUID_VERSION\u003e=1.1.7.\r\n\r\nI guess (a) suffices for now, but (b) would be more future proof in case we have to adjust the threshold ever again. Come to think of it, we could run into future problems with other libraries as well. Scattering the threshold and version checks across Makefile.sub and configure.ac for each should be avoided. I.e. I'd merge either (a) or (b), but have a slight preference for (b) in terms of setting a precedent for future patches."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/42#issuecomment-395023503"}}}</script> <script type="application/ld+json">{ "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "37567f93-e2a7-4e2a-ad37-a9160fc62647", "title": "Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)", "sections": [ { "text": "", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 42,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 42\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42#issuecomment-395023503" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 341834315\n}" } ], "themeColor": "26292E" }</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

Stefan Westerfeld-2
In reply to this post by Stefan Westerfeld-2

@swesterfeld pushed 3 commits.

  • 98f0cb7 AF-TESTS: add version comparision helper script
  • 9a844bb BUILD: provide fluidsynth version instead of test threshold to af-tests
  • 572be9b AF-TESTS: compute soundfont test threshold based on fluidsynth version


You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.

<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","potentialAction":{"@type":"ViewAction","target":"https://github.com/tim-janik/beast/pull/42/files/017557a2d2307b4816c36dc4a8e2386f6a3c62f7..572be9b14df54fa2af5aa3c9fcc5455bc1e7f529","url":"https://github.com/tim-janik/beast/pull/42/files/017557a2d2307b4816c36dc4a8e2386f6a3c62f7..572be9b14df54fa2af5aa3c9fcc5455bc1e7f529","name":"View Pull Request"},"description":"View this Pull Request on GitHub","publisher":{"@type":"Organization","name":"GitHub","url":"https://github.com"}}</script> <script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@swesterfeld pushed 3 commits in #42"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/42/files/017557a2d2307b4816c36dc4a8e2386f6a3c62f7..572be9b14df54fa2af5aa3c9fcc5455bc1e7f529"}}}</script> <script type="application/ld+json">{ "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "@swesterfeld pushed 3 commits in #42", "sections": [ { "text": "3 new commits pushed to tim-janik/beast #42:", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ { "name": "98f0cb7", "value": "AF-TESTS: add version comparision helper script" }, { "name": "9a844bb", "value": "BUILD: provide fluidsynth version instead of test threshold to af-tests" }, { "name": "572be9b", "value": "AF-TESTS: compute soundfont test threshold based on fluidsynth version" } ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42/files/017557a2d2307b4816c36dc4a8e2386f6a3c62f7..572be9b14df54fa2af5aa3c9fcc5455bc1e7f529" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 341834315\n}" } ], "themeColor": "26292E" }</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

Stefan Westerfeld-2
In reply to this post by Stefan Westerfeld-2

I see that it would be nicer to have the threshold next to the actual test. I implemented (b), so the version check is now in af-tests/Makefile.sub, using a new helper script based on sort -V - but I doubt we will have this problem often.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","potentialAction":{"@type":"ViewAction","target":"https://github.com/tim-janik/beast/pull/42#issuecomment-396259069","url":"https://github.com/tim-janik/beast/pull/42#issuecomment-396259069","name":"View Pull Request"},"description":"View this Pull Request on GitHub","publisher":{"@type":"Organization","name":"GitHub","url":"https://github.com"}}</script> <script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@swesterfeld in #42: I see that it would be nicer to have the threshold next to the actual test. I implemented (b), so the version check is now in af-tests/Makefile.sub, using a new helper script based on sort -V - but I doubt we will have this problem often."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/42#issuecomment-396259069"}}}</script> <script type="application/ld+json">{ "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)", "sections": [ { "text": "", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 42,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 42\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42#issuecomment-396259069" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 341834315\n}" } ], "themeColor": "26292E" }</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

Gnome - Beast mailing list
In reply to this post by Stefan Westerfeld-2

Closed #42.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Closed #42."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/42#event-1753713490"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/42#event-1753713490", "url": "https://github.com/tim-janik/beast/pull/42#event-1753713490", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)", "sections": [ { "text": "", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42#event-1753713490" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 341834315\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)

Gnome - Beast mailing list
In reply to this post by Stefan Westerfeld-2

Thanks, merged, plus I re-added your comments.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tim-janik in #42: Thanks, merged, plus I re-added your comments."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/42#issuecomment-407911435"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/42#issuecomment-407911435", "url": "https://github.com/tim-janik/beast/pull/42#issuecomment-407911435", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [tim-janik/beast] Fix af-tests/soundfont-test on bionic (#42)", "sections": [ { "text": "", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 42,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/42#issuecomment-407911435" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 341834315\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast