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

Time calculation in the "IT" blocks is broken in Forth when the tests crash #1

Closed
FArekkusu opened this issue Jul 26, 2019 · 24 comments
Assignees
Labels
bug Something isn't working

Comments

@FArekkusu
Copy link

A picture worth 1000 words:

Screenshot from 2019-07-26 11-30-22

@FArekkusu FArekkusu changed the title Time calculation in the "IT" blocks is broken in Forth Time calculation in the "IT" blocks is broken in Forth when the tests crash Jul 26, 2019
@Voileexperiments
Copy link

This should go to the runner repo.

Also a known issue: if you write past array boundary in C katas, you corrupt the address used to denote test time and get absurd time too.

@kazk kazk transferred this issue from codewars/codewars.com Jul 26, 2019
@kazk
Copy link
Member

kazk commented Jul 26, 2019

Forth is work in progress. We decided to release early so anyone interested can help.

@kazk kazk added the bug Something isn't working label Jul 26, 2019
@nomennescio
Copy link
Collaborator

I made a fix and committed, how to check if it works on Codewars?

@kazk
Copy link
Member

kazk commented Jul 26, 2019

? You can check if it's working by running the code on your machine.

Codewars doesn't do anything special for Forth and it just displays the output based on the test result.

@kazk
Copy link
Member

kazk commented Jul 26, 2019

Change the code in examples folder and try it with failing test.

@kazk
Copy link
Member

kazk commented Jul 26, 2019

If you can write tests for this extension to test the output, we can set up a CI. You can write tests using whatever language or framework is convenient, just test the contents of stdout.

Coderunner is just executing gforth preloaded.4th solution.4th ttester-codewars.4th tests.4th -e bye like in example/run-tests.sh.

@nomennescio
Copy link
Collaborator

Tests added

@nomennescio
Copy link
Collaborator

Fix in calculation of expected and actual results

@nomennescio
Copy link
Collaborator

@kazk I've written the tests and framework as you suggested, can you have a look at it? (I don't know what CI stands for, but I suppose it's Codewars testframe infrastructure)

@kazk
Copy link
Member

kazk commented Jul 29, 2019

I'll take a look later.

CI stands for Continuous Integration. We can set up Circle CI or Travis CI so that tests are run when opening PRs and after merging.

We'll close this when the changes are deployed.

@kazk kazk reopened this Jul 29, 2019
@kazk
Copy link
Member

kazk commented Jul 29, 2019

@nomennescio I've set up Circle CI. From now on, can you open a Pull Request instead of pushing directly to master? It's easier to keep track of the changes this way. It'll run the tests before merging and we can talk about the changes more easily.
You can still merge without my permission if the changes are trivial and doesn't need my review.

I'm assuming you made changes so @FArekkusu's code doesn't mess up the time. Which test file is this in?

@nomennescio
Copy link
Collaborator

So for the pull requests I would need to create a feature branch? Or a separate branch from which to repeatedly pull? Locally I use a branch from which I squash merge into master.

@nomennescio
Copy link
Collaborator

The test files test all supported features of the ttester framework and its Codewars interactions. The reported issue was about the number of returned (integer) results not being equal to the number of expected results. These are explicitly tested in:

  • test-stack-mismatch.4th
  • test-too-little-arguments.4th
  • test-too-many-arguments.4th

@nomennescio
Copy link
Collaborator

@kazk I've run into the dreaded auto-retire feature, so with only 3 users having "completed" my Forth Kata, and two issues reported due to an issue with the test framework, the "First Forth Kata" (https://www.codewars.com/kata/5d2d4796e3ab9300257343e9/edit/forth) which was the base of this reported issue with the test framework, is not available to beta tested by others anymore, even though I have fixed all technical issues. What can I/you do about it?

@kazk
Copy link
Member

kazk commented Jul 30, 2019

@nomennescio I'd recommend forking this repository. Then work in a branch for each change you're making, push the branch to your fork, and then open a PR to this repository.

# 1. Fork this repo on GitHub
# 2. Clone your fork
git clone [email protected]:nomennescio/ttester-codewars.git
cd ttester-codewars
# 3. Set upstream to this repo
git remote add upstream [email protected]:Codewars/ttester-codewars.git
# 4. Work in a branch per change
git checkout -b some-change
git add some-file.4th
git commit
# 5. Push to your fork
git push -u origin some-change
# 6. Open a PR
# 7. When merged, sync the changes by pulling from upstream and pushing to your fork
git checkout master
git pull upstream master
git push

This is very common (standard?) workflow when collaborating on GitHub so there's resources online if you search.


For the kata, you should add a Forth version to the Multiply kata after this change is deployed. We don't need a separate Forth version for it.

Try translating existing kata first. I also don't think we can test anything Forth specific with currently very limited test capabilities, but I can be wrong.

@nomennescio
Copy link
Collaborator

nomennescio commented Jul 30, 2019

As for translation, I already translated a different Kata, but as with all translations, you never know when it will get approval
https://www.codewars.com/kumite/5d40404391ee870028df1899?sel=5d40404391ee870028df1899

For testing purposes the current framework is good enough, given that integers are the dominant data type in Forth.

@kazk
Copy link
Member

kazk commented Jul 30, 2019

Why did you decide to translate that? I'm not approving it because it doesn't fit the kata. That one specifically says it's for C and have defined restrictions for it. Those don't make sense for Forth and translation doesn't test the requirements either.

@nomennescio
Copy link
Collaborator

Oh well, let's translate Multiply then.

@nomennescio
Copy link
Collaborator

@kazk
Copy link
Member

kazk commented Jul 30, 2019

All random tests expects 0.

image

@kazk
Copy link
Member

kazk commented Jul 31, 2019

Deployed the changes

@kazk kazk closed this as completed Jul 31, 2019
@nomennescio
Copy link
Collaborator

@kazk Yet another Kata translated
https://www.codewars.com/kumite/5d45d344cfc30d4fc3e79b88?sel=5d45d344cfc30d4fc3e79b88

this one is interesting in that it uses Forth redefinition of words and explicit loading of the solution.

@kazk
Copy link
Member

kazk commented Aug 3, 2019

@nomennescio please don't use GitHub issues like this. It's off topic. Use Gitter to ask others to review or wait a week so it shows up under pending translations tab enabled for some users.
For this one, I've reviewed and left a feedback.

@nomennescio
Copy link
Collaborator

@kazk right. I was just referencing it here as Forth Katas act as a test bed for the test framework, and currently there are only two approved Forth Katas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants