Skip to content

Fix CI regression test-fast skipped tests except on Windows#2342

Merged
EliahKagan merged 1 commit intoGitoxideLabs:mainfrom
EliahKagan:fix-ci-regression
Jan 4, 2026
Merged

Fix CI regression test-fast skipped tests except on Windows#2342
EliahKagan merged 1 commit intoGitoxideLabs:mainfrom
EliahKagan:fix-ci-regression

Conversation

@EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Jan 4, 2026

I had failed to remove an if guard that was meant to be removed in a refactoring, in c1a3a99 (#2337). That made it so test-fast skips running the tests on non-Windows systems and just reports success!

This fixes that serious CI bug by removing the left-over if:.


I noticed this in #2341 by observing that some tests passed when they shouldn't have been able to, and very fast.

As for this PR, I think everything should pass, but the bug also made it so that the changes to test-fast was not adequately tested before merging #2337, so that's not guaranteed.

I had failed to remove an `if` guard that was meant to be removed
in a refactoring, in c1a3a99 (GitoxideLabs#2337). That made it so `test-fast`
skips running the tests on non-Windows systems and just reports
success!

This fixes that serious CI bug by removing the left-over `if:`.
@EliahKagan

This comment was marked as outdated.

Copy link
Member Author

@EliahKagan EliahKagan left a comment

Choose a reason for hiding this comment

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

Okay. I've checked all three of the non-Windows test-fast jobs here to verify they actually run the cargo nextest step, and also that they are passing.

This should be ready to merge so long as everything else passes.

Also: Sorry about that!

@EliahKagan EliahKagan marked this pull request as ready for review January 4, 2026 12:56
@EliahKagan EliahKagan enabled auto-merge January 4, 2026 12:56
@EliahKagan EliahKagan merged commit 9cb6b22 into GitoxideLabs:main Jan 4, 2026
30 checks passed
@EliahKagan EliahKagan deleted the fix-ci-regression branch January 4, 2026 13:11
@EliahKagan EliahKagan requested a review from Copilot January 4, 2026 18:14

This comment was marked as outdated.

@Byron
Copy link
Member

Byron commented Jan 5, 2026

Also: Sorry about that!

No worries about that!
I think using Copilot as safety net is the way to go and I am doing what I preach for a couple of weeks already. Now I'd think it catches most of my slop.

Regarding the "Would it have caught that stray if condition" (and I think it didn't), then I recommend marking such lines for testing clearly and it will definitely find it.

This is where Azure and Copilot should really shine and I'd hope we can use more of it. That might mean that you can use it more for running research jobs, many of which will come out empty or garbage, but some of which will be successful and still safe time overall. At least this is how I use it, besides reviews, and it's good for that.

@EliahKagan
Copy link
Member Author

I think using Copilot as safety net is the way to go

I agree--we should use that together with other practices and scanning techniques, and it is often capable of catching problems of this general kind, even when other measures don't even approach doing so.

However, in this case, I think the problem is harder for Copilot to notice than it seems.

Regarding the "Would it have caught that stray if condition" (and I think it didn't), then I recommend marking such lines for testing clearly and it will definitely find it.

The problem is that the disconnect between how I did mark this and my own intepretation of how I marked it was the mistake. I commented that having two sections with separate conditionals was bad and needed to be refactored. That was in the code before the refactor. Then, when refactoring, I removed the comment and one section, while modifying the other section as needed, except that I failed to remove its if.

Nonetheless...

This is where Azure and Copilot should really shine and I'd hope we can use more of it.

I agree with this in general.

Regarding Azure, some AI-related services are nontrivial to set up. As you know, there was an opportunity a while back for me to get help with that, but unfortunately I didn't have the time to make that happen. But I think I'll probably still be able to get everything set up.

For setting up Azure Pipelines to complement GitHub Actions, I don't expect any big problems, though there may be more steps to go through if we need GPU runners for anything.

@Byron
Copy link
Member

Byron commented Jan 8, 2026

Regarding Azure, some AI-related services are nontrivial to set up. As you know, there was an opportunity a while back for me to get help with that, but unfortunately I didn't have the time to make that happen. But I think I'll probably still be able to get everything set up.

Somehow I still have a feeling that one could get such help even once the initial period is expired. It's probably worth an attempt once you know what you are after. I wouldn't even know what to ask.
The only thing I find truly interesting is to use Azure credits for more Organisation features/more Copilot, with the note that I never ran out of Copilot credits, but out of 'concurrency'. Now this seems a bit better though, as I can easily run two tasks at the same time.

Overall, all good I think.

@EliahKagan
Copy link
Member Author

EliahKagan commented Jan 8, 2026

One thing I want to look into is whether we can use our Azure credits to pay for GitHub-related services. My impression is that we could, but while it's definitely possible to pay for them through Azure, I'm not 100% sure the credits would get used. I'll definitely want to look into that first! (For things on Azure, I'm much less worried that any such surprises could occur--please do use the Azure services anytime you have a use for them.)

Regarding Copilot, does it show you as having a Pro subscription or as having a Pro+ subscription? We both have Copilot provided by GitHub in connection with our work on gitoxide, but we actually have it through different mechanisms if I underrstand correctly, so there's no reason to expect it works the same for both of us. It shows as Pro+ for me, but there seem also to be bugs, or ways I don't understand how premium requests work, such that Copilot pull requests always fail for me on the grounds that I'm out of premium requests, even as other Copilot features continue to work. (With that said, I don't know all other Copilot features work--I mostly just use the chat interface, on the website.)

@Byron
Copy link
Member

Byron commented Jan 8, 2026

Yes, I am afraid of a billing-messup as well. It's complicated, and buggy for sure.

And regarding the subscription, that's interesting, it shows as GitHub Copilot Pro for me and at least I don't have any issues unless I try to launch too many. Sometimes two at a time work, often not. Maybe that's buggy as well.

If in doubt, I wouldn't change a thing, but it would certainly be interesting to know if there is potential to fix it. For instance, if the billing goes through the organisation, I am pretty sure it would have to allow more parallelism.

@EliahKagan
Copy link
Member Author

It may be possible to enable Copilot for the organization itself on this page. However, when I try to do that, it gives an error without much detail. It may be that wiring up Azure to it would fix that, but that goes back to being the situation where I'd want to look into it first to avoid charges that bypass the credits.

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