Skip to content
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

Fix unit tests and enable tests in CI for ai-video #3190

Merged
merged 27 commits into from
Oct 21, 2024

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Oct 2, 2024

Changes:

@leszko leszko changed the base branch from master to ai-video October 2, 2024 08:40
@leszko leszko changed the title Fix unit tests in ai-video Fix unit tests and enable tests in CI for ai-video Oct 3, 2024
@leszko leszko marked this pull request as ready for review October 3, 2024 11:06
@leszko leszko requested a review from rickstaa as a code owner October 3, 2024 11:06
@yondonfu
Copy link
Member

yondonfu commented Oct 3, 2024

Re: removing tests for the deprecated net.SegData.Profiles field.

I think it is fine to remove the tests and to keep the commit.

IIRC the problem I was trying to solve with that commit was that the AI requests would trigger a failure on the O for the call common.BytesToVideoProfile(segData.Profiles) because the B sets the net.SegData.Profiles field to []byte("invalid") (the NetSegData() function is used in server.genSegCreds() in server.prepareAIPayment()).

And IIRC the reason for keeping the logic for parsing the Profiles field was to maintain compatibility with older versions of go-livepeer - a G on an older version of go-livepeer prior to the introduction of the new profile related fields in net.SegData would still be able to speak with an O on a newer version of go-livepeer after the introduction of those fields. But, its been awhile and IMO at some point it makes sense to cleanup this really old deprecated field if no one is running such an old version of go-livepeer.

I would just recommend announcing with plenty of lead time that this will be a breaking change unlikely to affect anyone, but if you are running a old version of go-livepeer for a G you should upgrade to a more recent one that uses the other profile fields of net.SegData.

@leszko leszko requested a review from rickstaa October 15, 2024 13:02
server/selection_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had some questions but apart from that looks good to me.

server/broadcast.go Outdated Show resolved Hide resolved
@leszko leszko merged commit d5f0db7 into ai-video Oct 21, 2024
13 of 14 checks passed
@leszko leszko deleted the rafal/ai-video-fix-unit-tests branch October 21, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants